FyroxEngine / Fyrox

3D and 2D game engine written in Rust
https://fyrox.rs
MIT License
7.49k stars 340 forks source link

#630 transmute_vec_as_bytes potential fix #662

Closed NathanaelG1 closed 4 days ago

NathanaelG1 commented 1 week ago

Hey all new here but believe this may fix the issue raised in #630. Added some comments on the issue thread including a playground which shows the result of the old function versus the new function. I have taken an approach to only account for the specific types which actually are used in the codebase (f32 and usize) as there were a lot of issues dealing with a totally generic type. I understand this adds a limitation that was previously not there, if this is not a good approach let me know and I can do some reformatting.

I have added two unit tests for transmute_vec_as_bytes which makes sure the results do not include any random values from the padding.

I removed the need for an unsafe block because the types are known to the function, and did not add any dependencies to the project.

All unit tests are passing, and running my modified tutorials from the RPG and FPS on the documentation seem to run the same as on the master branch of the main fork. This is my first PR here if there's anything I can do to improve it let me know!

mrDIMAS commented 1 week ago

Hi, this looks like a partial solution. May I ask you, could you please try using bytemuck crate for this case? Afaik all you need to do is to add Pod trait bound to T and this should automagically fix the issue.

NathanaelG1 commented 5 days ago

@mrDIMAS

I have added bytemuck and applied the Pod trait to T. During compilation, an error indicated that Pod should be implemented for BlendShapesContainer::from_lists::VertexData. When I attempted to derive Pod for VertexData, additional errors surfaced for each Vec3 declared in the implementation block.

Using bytemuck might require significant changes to the codebase. At a minimum, bytemuck would need to be added to both fyrox-core and fyrox-impl, and a Pod wrapper with implementations of Pod and Zeroable would need to be created for VertexData.

If this approach sounds acceptable, I'll proceed with the necessary changes.

errors: --> fyrox-impl/src/scene/mesh/surface.rs:170:21 170 let bytes = crate::core::transmute_vec_as_bytes(vertex_data); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter Pod declared on the function transmute_vec_as_bytes
--> fyrox-impl/src/scene/terrain/mod.rs:116:9 116 crate::core::transmute_vec_as_bytes(height_map), ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type of the type parameter Pod declared on the function transmute_vec_as_bytes
mrDIMAS commented 5 days ago

Yes, you can proceed. As for nalgebra items (Vectors, etc) - you can add bytemuck feature to nalgebra crate, and it will implement the required traits.

NathanaelG1 commented 4 days ago

Made the suggested changes and left the unit tests.