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

Improve safety of bevy_mikktspace #7372

Open
LaylBongers opened this issue Jan 26, 2023 · 4 comments · May be fixed by #9050
Open

Improve safety of bevy_mikktspace #7372

LaylBongers opened this issue Jan 26, 2023 · 4 comments · May be fixed by #9050
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Dependencies A change to the crates that Bevy depends on P-Unsound A bug that results in undefined compiler behavior

Comments

@LaylBongers
Copy link

What problem does this solve or what need does it fill?

I'm the original author of the port of the mikktspace library to generated Rust code. While this originally solved its purpose of making the crate easier to compile to WASM targets without the need for a C compiler, the auto-generated code with heavy manual fixes was always meant to be replaced eventually. See:

//! Everything in this module is pending to be refactored, turned into idiomatic-rust, and moved to
//! other modules.

What solution would you like?

I do not think that unsafety is necessary for the majority of this algorithm, its maintainability can be improved without a bit hit on performance by starting to port more sections of it to idiomatic rust, with appropriate benchmarks and tests to validate the process.

What alternative(s) have you considered?

Re-implementing from scratch isn't particularly useful as it's a small enough surface area where a gradual port would work better. The original crate does still exist too and improvements could be made there, I couldn't quite find the reason for the fork, was this just to make it easier to maintain?

@LaylBongers LaylBongers added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 26, 2023
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change P-Unsound A bug that results in undefined compiler behavior C-Dependencies A change to the crates that Bevy depends on and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jan 26, 2023
@alice-i-cecile
Copy link
Member

Great to hear from you :) IIRC we ended up forking because of difficulties synchronizing glam versions. If you're interested in adding one or two folks from Bevy to the maintainers of mikktspace we may be able to unfork and make improvements upstream.

The safety work was started in #4932 by @DJMcNab, although it was pretty slow going. An agressive test suite would make me much more comfortable with any changes made.

@LaylBongers
Copy link
Author

I'm not on the team maintaining the mikktspace crate so I don't think I can add maintainers, but I can take a look at adding some tests and benchmarks to bevy's fork definitely.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 26, 2023

I suspect it's worthwhile running a newer C2Rust on the original again, before we start.

And some comparative fuzzing against a compiled from C version can't go amiss

@LaylBongers
Copy link
Author

That might be a good idea, but, a note on that:
The original code didn't work straight up out of the box after conversion, it needed a lot of additional work. The "cube.obj" example I included is what I used for validation at the time. I removed the sections of code that were just fallbacks for being unable to malloc (under the maybe incorrect assumption that failing to alloc is a panic anyways). I also converted some raw mallocs with equivalent vectors. A straight re-conversion will lose these manual adjustments so it's not a 1:1 easy thing to do.

@LaylBongers LaylBongers linked a pull request Jul 5, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Dependencies A change to the crates that Bevy depends on P-Unsound A bug that results in undefined compiler behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants