Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Come up with a better way to pass strings into the bindings. #6

Closed
floooh opened this issue Mar 30, 2023 · 6 comments
Closed

Come up with a better way to pass strings into the bindings. #6

floooh opened this issue Mar 30, 2023 · 6 comments

Comments

@floooh
Copy link
Owner

floooh commented Mar 30, 2023

This doesn't look very nice:

 let window_title = b"mrt\0".as_ptr() as _;

If that's the only way to define a C string in Rust then maybe it would make sense to create 2 structs in the bindings, one internal struct with C compatible types, and a separate public struct with 'Rust-y' types. The wrapper function would then create a temporary C-compatible struct, copy the values from the public struct over and do any type conversion.

@floooh
Copy link
Owner Author

floooh commented Mar 30, 2023

...look at imgui-rs for inspiration:

https://github.com/imgui-rs/imgui-rs/blob/main/imgui/src/string.rs

...sokol-rust can probably be easier it can temp-allocate before the C function is called, and free right after, because the sokol headers copy the data internally if needed.

@EriKWDev
Copy link
Contributor

EriKWDev commented Apr 2, 2023

Yes. I partially did this for functions that took a string as a parameter.

These functions will have a &str as signature and than inside the function temporarily allocate a CString and pass it to the ffi functions.

The reason I didn't do this for the structs (such as where the window title is needed^) was that it would become a bit more complicated since we would have to do this in all functions that take a struct that holds a string. This is of course possible but was a bit more bookkeeping than I was willing to think about for the initial bindings xP

@waywardmonkeys
Copy link
Contributor

In #25, Floh commented:

Thanks!

Btw, I noticed that Rust 1.77 has proper C string literals now via c"Hello World", but simply plugging those in doesn't work, because those literals resolve to &CStr, while the current sokol-rust bindings currently use a c_char pointer (*const core::ffi::c_char).

Since you have more Rust experience than me, do you know if allowing something like this would be an 'easy fix'?

    sapp::run(&sapp::Desc {
        // ...
        window_title: c"Hello World",
        // ...
        ..Default::default()
    });

...instead of the currently very awkward:

    sapp::run(&sapp::Desc {
        // ...
        window_title: b"clear\0".as_ptr() as _;
        // ...
        ..Default::default()
    });

I've been using this syntax in other crates and it is very nice.

Trying this out in this repo, this compiles and runs on macOS:

--- a/examples/cube/shader.rs
+++ b/examples/cube/shader.rs
@@ -342,42 +342,42 @@ pub fn cube_shader_desc(backend: sg::Backend) -> sg::ShaderDesc {
     let mut desc = sg::ShaderDesc::new();
     match backend {
         sg::Backend::Glcore33 => {
-            desc.attrs[0].name = b"position\0".as_ptr() as *const _;
-            desc.attrs[1].name = b"color0\0".as_ptr() as *const _;
+            desc.attrs[0].name = c"position".as_ptr();
+            desc.attrs[1].name = c"color0".as_ptr();
             desc.vs.source = &VS_SOURCE_GLSL330 as *const _ as *const _;
-            desc.vs.entry = b"main\0".as_ptr() as *const _;
+            desc.vs.entry = c"main".as_ptr();
             desc.vs.uniform_blocks[0].size = 64;
             desc.vs.uniform_blocks[0].layout = sg::UniformLayout::Std140;
-            desc.vs.uniform_blocks[0].uniforms[0].name = b"vs_params\0".as_ptr() as *const _;
+            desc.vs.uniform_blocks[0].uniforms[0].name = c"vs_params".as_ptr();
             desc.vs.uniform_blocks[0].uniforms[0]._type = sg::UniformType::Float4;
             desc.vs.uniform_blocks[0].uniforms[0].array_count = 4;
             desc.fs.source = &FS_SOURCE_GLSL330 as *const _ as *const _;
-            desc.fs.entry = b"main\0".as_ptr() as *const _;
-            desc.label = b"cube_shader\0".as_ptr() as *const _;
+            desc.fs.entry = c"main".as_ptr();
+            desc.label = c"cube_shader".as_ptr();
         },
         sg::Backend::D3d11 => {
-            desc.attrs[0].sem_name = b"TEXCOORD\0".as_ptr() as *const _;
+            desc.attrs[0].sem_name = c"TEXCOORD".as_ptr();
             desc.attrs[0].sem_index = 0;
-            desc.attrs[1].sem_name = b"TEXCOORD\0".as_ptr() as *const _;
+            desc.attrs[1].sem_name = c"TEXCOORD".as_ptr();
             desc.attrs[1].sem_index = 1;
             desc.vs.source = &VS_SOURCE_HLSL4 as *const _ as *const _;
-            desc.vs.d3d11_target = b"vs_4_0\0".as_ptr() as *const _;
-            desc.vs.entry = b"main\0".as_ptr() as *const _;
+            desc.vs.d3d11_target = c"vs_4_0".as_ptr();
+            desc.vs.entry = c"main".as_ptr();
             desc.vs.uniform_blocks[0].size = 64;
             desc.vs.uniform_blocks[0].layout = sg::UniformLayout::Std140;
             desc.fs.source = &FS_SOURCE_HLSL4 as *const _ as *const _;
-            desc.fs.d3d11_target = b"ps_4_0\0".as_ptr() as *const _;
-            desc.fs.entry = b"main\0".as_ptr() as *const _;
-            desc.label = b"cube_shader\0".as_ptr() as *const _;
+            desc.fs.d3d11_target = c"ps_4_0".as_ptr();
+            desc.fs.entry = c"main".as_ptr();
+            desc.label = c"cube_shader".as_ptr();
         },
         sg::Backend::MetalMacos => {
             desc.vs.source = &VS_SOURCE_METAL_MACOS as *const _ as *const _;
-            desc.vs.entry = b"main0\0".as_ptr() as *const _;
+            desc.vs.entry = c"main0".as_ptr();
             desc.vs.uniform_blocks[0].size = 64;
             desc.vs.uniform_blocks[0].layout = sg::UniformLayout::Std140;
             desc.fs.source = &FS_SOURCE_METAL_MACOS as *const _ as *const _;
-            desc.fs.entry = b"main0\0".as_ptr() as *const _;
-            desc.label = b"cube_shader\0".as_ptr() as *const _;
+            desc.fs.entry = c"main0".as_ptr();
+            desc.label = c"cube_shader".as_ptr();
         },
         _ => {},
     }

@waywardmonkeys
Copy link
Contributor

So if you want, I can do this across the repo and submit a PR.

@floooh
Copy link
Owner Author

floooh commented Mar 27, 2024

Ah I see, so the new form would be c"bla".as_ptr();, yeah, looks good! (PS: for later I wonder if we can also get rid of the .as_ptr()... hmm)

If you want to provide a PR to fix the examples, please don't bother with the shader.rs files since those are code-generated via sokol-shdc anyway. But a fix for the actual example sources would be much appreciated :)

I'm currently working on sokol-shdc in a branch anyway and will integrate the change there as a 'drive-by fix', and then convert the shader.rs files in the sokol-rust repo once that PR is merged (might take a little while, since the actual reason for the PR is storage-buffer support).

@waywardmonkeys
Copy link
Contributor

Already did the shader files ... just 2 search / replace functions. :)

I'll finish up and submit the PR tomorrow though as it is late here (and should also update docs to say that the samples require Rust 1.77).

@floooh floooh closed this as completed in d01406e Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants