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

VertexStream::new_with_stride() pointer parameter leads to easy segfault #52

Open
JMS55 opened this issue Oct 12, 2024 · 9 comments
Open

Comments

@JMS55
Copy link
Contributor

JMS55 commented Oct 12, 2024

What I tried doing (and got a segfault with):

VertexStream::new_with_stride::<Vec3, _>(
    &vertex_buffer, // vertex_buffer is a Vec<u8>
    vertex_stride,
)

What you actually need to do:

VertexStream::new_with_stride::<Vec3, _>(
    vertex_buffer.as_ptr(),
    vertex_stride,
)

I suggest changing the function to not take a raw pointer.

The whole T vs VertexType thing was also confusing to me, it took me a while to figure out how to use the function. I'd prefer simply passing the size myself rather than it using size_of::<T>().

@Uriopass
Copy link
Collaborator

What would you suggest as a proper API?
This would lead to a breaking change too, which is fine but the next release is probably not anytime soon.
I think this function should be marked unsafe too, it seems easy to cause UB.

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 30, 2024

Probably take an &[T] instead of a raw pointer.

@Uriopass
Copy link
Collaborator

but since T are stored with strides: eg a slice of [T, U, T, U, T, U]
I think that would be UB to make the slice

My idea would be to simply take *const u8 and acknowledge the unsafeness. I'm not sure how stride can be encoded safely and VertexStream isn't used in many functions

@JMS55
Copy link
Contributor Author

JMS55 commented Nov 30, 2024

Oh, you're right :/. Would &[u8] be an issue? Or *const u8 would probably be fine, but I want something that makes passing &Vec<u8> (pointer to the vec structure itself on the stack, not the vec's allocated memory) impossible.

@Uriopass
Copy link
Collaborator

Uriopass commented Nov 30, 2024

&[u8] could be a good idea! I guess users would use bytemuck which would solve a few problems with getting this right along with a nice lifetime

Also starts to look a lot like a static VertexDataAdapter

@Uriopass
Copy link
Collaborator

Opened #60 if you want to take a look

@Uriopass
Copy link
Collaborator

Uriopass commented Dec 22, 2024

@JMS55 how are you using the stream? Now that I'm reading the API more carefully I'm not sure if it's possible to have an offset inside a vertex struct using a slice. I'm also not sure how meshopt knows where the stream ends and I don't understand the "stream" name since it's finite really.

@JMS55
Copy link
Contributor Author

JMS55 commented Dec 22, 2024

I use it to get a position-only stream of the vertices https://github.com/bevyengine/bevy/blob/main/crates/bevy_pbr/src/meshlet/from_mesh.rs#L98-L101.

vertex_buffer is a Vec<u8>.

@Uriopass
Copy link
Collaborator

This only works because position is the first element :/ I'm not sure how it'd look if you only wanted scale or rotation for example
I think this should be VertexDataAdapter but at the same time meshopt is made with this stream thing.. Not sure if it's better to encapsulate or just leave it raw without the footgunny size_of

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

2 participants