Refactor bevy_mikktspace to entirely safe rust

[LaylBongers]

trafficstars

Objective

4 years ago, with a heavy amount of hand-translation, I converted mikktspace.c to pure-rust so I could load GLTF files in-browser. I had intended this to be a quick and hacky conversion, to be gradually refactored over time, but to my surprise it's still in use in almost its original unsafe form! No more!

Fixes #7372

Solution

This pull requests finally converts the remaining unsafe code to safe rust, fixes rustc warnings, and most clippy warnings. I also re-added some comments that had gotten lost in the translation, and re-added flags.

The algorithm is still the same, the biggest changes made change it to use relative offsets into buffers instead of direct pointers.

[github-actions[bot]]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[DJMcNab]

A couple of results from my prior attempt (https://github.com/bevyengine/bevy/pull/4932) - this really needs proper testing. That is, comparisons that it gets exactly the same results as the original C library. That should be both a wide range of known examples, and probably fuzzing (although the original C code does not have principled NaN handling, so we need to be careful there) Certainly that we have the one test is good, but a full rewrite comes a much higher risk of disagreements here

I also don't particularly like that we own this code, although I also realise that there's not really any other potential steward. Because of the testing requirements, it might make more sense for it to live in a different repo in this organisation, although working out the release process there is more awkward.

[LaylBongers]

I definitely agree with this, we need a test bench that validates the Rust port against references with non-trivial models generated using the original C library.

[LaylBongers]

I have just now added a reference generator to the PR, including result files from said generator. This is not yet used currently, but this will be the basis of new reference tests to make sure the Rust implementation of MikkTSpace gives identical results.

[LaylBongers]

Regression tests validated against the original C are now included!

[LaylBongers]

Squashed and cleaned, this should now be ready for review and merge!

[mockersf]

do you have an idea perf-wise how it compares to the unsafe version?

[LaylBongers]

I have not done a benchmark check against either the unsafe translation, or the original C. I'd imagine it's not a huge difference given that the implementation hasn't actually changed at all. There will be additional checks as array accesses are bounds checked.

[janhohenheim]

Fully agree with @NthTensor. It would be a shame to let this accumulate more merge conflicts. Maybe we could download the blobs on CI test runs and let the tests pass locally if the blobs were not found? @BD103 Added it to the 0.15 milestone for consideration; feel free to remove it again if you disagree.

[alice-i-cecile]

I also want this, but we need to do some requirements gathering and collaboration. IMO a working group is likely to be the best way to get this into shape.

[janhohenheim]

@alice-i-cecile hmm? I fail to see why we wouldn't just want to merge it. The version in this PR is

• made by the person who wrote the original code
• byte equivalent to the reference implementation (!)
• contains regression tests
• runs the same algorithm

This seems like a straight-up complete upgrade. Am I missing something?

[alice-i-cecile]

Hmm, okay. I remember @superdump expressing reluctance in the past, but byte-identical behavior is very hard to argue with.

[BD103]

Maybe we could download the blobs on CI test runs and let the tests pass locally if the blobs were not found? @BD103

I'm not sure I understand what the blobs are used for in this circumstance. Does the generator C program generate all of the .bin files, and regression_test.rs checks that the Rust program matches the output of the C program?

[LaylBongers]

That's correct, the tests check that the code generates byte-identical results. I figured that's the most appropriate test for this conversion, which should perform where possible identical floating point operations in identical orders, and thus produce byte-identical results.

[atlv24]

do we want to fuzz test this to check nan and infinity handling is identical with C? also, do we want to commit C into the bevy repo?

[BenjaminBrienen]

I'm surprised that adding clang/cmake stuff is "uncontroversial".

[LaylBongers]

This is mainly the reason I included the binary files in git tree rather than generating them at test time, so as to not include CMake into the actual build at any point. Those files are only there to generate the comparison files, and do not take part in the regular build.

[rparrett]

What's needed to move this forward? It looks like:

• [ ] Quantify any performance regressions
• [ ] Either come to a consensus that adding 336KB of test data is OK, or perhaps move the test case generator and test data into a separate bevy org repo?

[janhohenheim]

For future readers: this was merged! It was just moved outside the monorepo :) See https://github.com/bevyengine/bevy_mikktspace