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

Native JS Float32Array binding to Vec<f32> #968

Closed
Roba1993 opened this issue Oct 16, 2018 · 10 comments
Closed

Native JS Float32Array binding to Vec<f32> #968

Roba1993 opened this issue Oct 16, 2018 · 10 comments

Comments

@Roba1993
Copy link

Hi,

first of all thank you for the great crate!
I actually deep dive into the WebGL2 bindings and in the documentation the following way to get Values to native Float32Array is mentioned:

    let vertices: [f32; 9] = [-0.7, -0.7, 0.0, 0.7, -0.7, 0.0, 0.0, 0.7, 0.0];
    let memory_buffer = wasm_bindgen::memory().dyn_into::<WebAssembly::Memory>().unwrap().buffer();
    let vertices_location = vertices.as_ptr() as u32 / 4;
    let vert_array = js_sys::Float32Array::new(&memory_buffer).subarray(
        vertices_location,
        vertices_location + vertices.len() as u32,
    );

I have the feeling, that the data is cloned twice here?
Additionally this syntax and conversion is really confusing, especially because JS is already handling a native f32.

Is there a way to improve?
If I can get some high level guidance, I would also try to implement it on my own.

Thanks,
Robert

@jsheard
Copy link
Contributor

jsheard commented Oct 16, 2018

This came up in #965, the buffer creation could be improved but js_sys is missing the necessary 3-parameter typed array constructor. We should be able to do something like

let vert_array = js_sys::Float32Array::new_with_offset_and_length(&memory_buffer, vertices.as_ptr() as u32, vertices.len() as u32);

which creates a view of wasm memory without copying it.

@alexcrichton
Copy link
Contributor

Thanks for the report! The first snippet here is using subarray which avoids copying data, so the vertices array isn't copied in memory and vert_array is just a sub-view into the same data. The syntax is indeed a bit confusing and hopefully won't always be necessary!

I've submitted #970 to bind more constructors to make it a bit moer ergonomic in this case.

@Roba1993
Copy link
Author

@alexcrichton Thank for this fast update! But I haven't spotted a f32 Array in your code commit. Is this up to come, or did I miss something here?

@alexcrichton
Copy link
Contributor

Oops I left those out by accident! So many arrays to update...

@Roba1993
Copy link
Author

Thanks for doing this :)

@Roba1993
Copy link
Author

Hey,

I tried the changes in my crate and change to the actual GitHub master branch. But was-bingen fails with the following message:
error: failed to extract wasm-bindgen custom sections | caused by: failed to decode what looked like wasm-bindgen data: missing field

Has anyone experienced the same? I would like to validate the code change from Alex and close this issue.

Thanks,
Roba

@jsheard
Copy link
Contributor

jsheard commented Oct 18, 2018

When you're using the git version of wasm-bindgen crates you also need to install the git version of wasm-bindgen-cli, otherwise the version mismatch usually breaks things. Try this:

cargo install wasm-bindgen-cli --force --git https://github.com/rustwasm/wasm-bindgen

@Roba1993
Copy link
Author

Roba1993 commented Oct 18, 2018

Okay guys, I go it and the code is now the following:

    let vertices: [f32; 9] = [-0.7, -0.7, 0.0, 0.7, -0.7, 0.0, 0.0, 0.7, 0.0];
    let memory_buffer = wasm_bindgen::memory().dyn_into::<WebAssembly::Memory>().unwrap().buffer();
    let vert_array = js_sys::Float32Array::new_with_byte_offset(&memory_buffer, vertices.as_ptr() as u32);

In my opinion we should make this even easier, like the following:

let vert_array = js_sys::Float32Array::new_from_array(&vertices);

What do you think?

@alexcrichton
Copy link
Contributor

@Roba1993 that was the goal of #918 and topic of #811 as well, and I think we can probably just do some more work on our end to make that possible!

(although if that's the final issue here we may want to close in favor of #811)

@Roba1993
Copy link
Author

Hi alex,

I think #811 is the other way around, but yes I'm with you.
I close this and continue to look at the magic which get created here.

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