Repository navigation
Improve safety of bevy_mikktspace #7372
Description
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?
Activity
alice-i-cecile commentedon Jan 26, 2023
Great to hear from you :) IIRC we ended up forking because of difficulties synchronizing
glamversions. If you're interested in adding one or two folks from Bevy to the maintainers ofmikktspacewe 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 commentedon Jan 26, 2023
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 commentedon 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 commentedon Jan 26, 2023
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.
Move bevy_mikktspace out of tree (#19798)
Merge 'main' into 'next-gen-scenes' (#43)