Refactor bevy_mikktspace to entirely safe rust

[LaylBongers]

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]

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 (#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.

[Elabajaba]

Would it be better (or more "bevy native" or w/e) to use glam Vec2/Vec3/Vec4s instead of [f32; 2/3/4] for the stuff in this trait? I'm not sure if it really matters, but the crate already relies on glam so it's not like that'd increase potential dependencies for non-bevy library users (who also aren't using glam).

[NthTensor]

I'd really like to get this moving forward, and ideally land this early in the 0.15 cycle. Considering that it's tested to be byte equivalent to the reference implementation, and only adds costs on startup/asset-load I think it's pretty much ready.

The only hitch is that this introduces several large binary blobs for testing, which I personally am against including in the repo. Is there a way we could commit this without the blobs?

[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.