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

#630 transmute_vec_as_bytes potential fix #662

Merged
merged 11 commits into from
Jul 4, 2024

Conversation

NathanaelG1
Copy link
Contributor

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!

Ouroboros and others added 4 commits June 22, 2024 18:41
chore(FyroxEngine#630): reformatting transmute_vec_as_bytes to properly handle padding for usize and f32 types
@mrDIMAS
Copy link
Member

mrDIMAS commented Jul 1, 2024

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
Copy link
Contributor Author

@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
Copy link
Member

mrDIMAS commented Jul 3, 2024

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
Copy link
Contributor Author

Made the suggested changes and left the unit tests.

fyrox-impl/src/scene/mesh/surface.rs Outdated Show resolved Hide resolved
@NathanaelG1 NathanaelG1 requested a review from mrDIMAS July 3, 2024 23:16
@mrDIMAS mrDIMAS merged commit 474e3b0 into FyroxEngine:master Jul 4, 2024
11 checks passed
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

Successfully merging this pull request may close these issues.

2 participants