Refactor bevy_mikktspace into idiomatic Rust

[DJMcNab]

Objective

• bevy_mikktspace's use of unsafe is concerning.
• Eventually, we want this to be #[forbid(unsafe_code)]
• The refactor needs to maintain the same behaviour.

Solution

• Do small refactoring on the unsafe version (e.g. moving variables down to their declarations, adding comments)
• Chip away at the unsafe code
• Refactor some of the gnarlier parts to use standard facilities

[alice-i-cecile]

Yes please. Fundamentally from what I can see there's nothing in this algorithm that should require the use of unsafe.

[alice-i-cecile]

I'm giving this the Unsound label. I haven't proven that there's UB in the original auto-translated code, I would be very very surprised if there was none. Even if there isn't, the safety hazards of having this code around in the current form are very high due to the extreme challenges working with the existing code correctly.

[DJMcNab]

bors try

Failure seems to be spurious?

Note to reviewers: This isn't used at all internally, so hopefully there are no behaviour changes, but I don't care significantly if there are :).

Good future work is adding comparison fuzzing with the original C code's output (note: not the Rust port, since that definitely has UB).
Calling C code which has UB from Rust might itself be UB - and original mikktspace apparently has UB. Therefore, we'd probably need to compile the original C to WASM for this?

[bors]

try

Build succeeded:

• build (stable, ubuntu-latest)
• markdownlint
• run-examples-on-wasm
• check-doc
• run-examples-on-windows
• build-android
• build (stable, windows-latest)
• build (stable, macos-latest)
• build (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• build-wasm (nightly, ubuntu-latest)
• run-examples
• check-missing-examples-in-docs
• check-unused-dependencies
• ci
• miri
• check-compiles
• build-and-install-on-iOS

[superdump]

I would like to review this before it is merged. Just mentioning that so it doesn’t get merged before I’ve taken a look.

I have not yet reviewed the PR so the following comments are not based on having seen the changes, just on awareness that this PR makes changes to numerically-sensitive code. I’m concerned about the numeric behaviour of any changes. I know @DJMcNab is aware of this concern and I trust that care has been taken. Still I feel it is easy to unintentionally change arithmetic behaviour. Ideally there would be a comprehensive set of regression tests for this kind of thing, as @DJMcNab suggests, comparing against the behaviour of the original the scope of verifying numeric results are the same. I suppose as we have the flight helmet model in the repo, one thing we could do is to generate tangents for that using the original C code and store those in the repo, and generate tangents using this code and compare. It wouldn’t be a set of tests targeted at edge/corner cases but it would at least test something.

Note to reviewers: This isn't used at all internally, so hopefully there are no behaviour changes, but I don't care significantly if there are :).

I don’t understand this comment. What isn’t used internally?

[DJMcNab]

So what I mean is my priority is removing the unsound implemenation more than having exactly the same behaviour as the C code.

As an examplel, the original C code is completely careless in their handling of NaN/Infinity, and so I'm fairly sure we have different behaviour there (more panicky). However, I think the original's behaviour would still be completely wrong anyway.
As far as I know, the only changes I've made to the behaviour are cases where NaN/Infinity (/maybe subnormals) crop up.

The vast majority of the code doesn't do any floating point handling, and is just moving data around in hilariously evil ways.
The parts that do floating point operations, I think I've done an exact translation.

[superdump]

So what I mean is my priority is removing the unsound implemenation more than having exactly the same behaviour as the C code.

How do you define behaviour? It is only the numeric results that I care about being consistent. How it gets there, aside from performance, code maintenance, API ergonomics, etc, is secondary.

As an examplel, the original C code is completely careless in their handling of NaN/Infinity, and so I'm fairly sure we have different behaviour there (more panicky). However, I think the original's behaviour would still be completely wrong anyway. As far as I know, the only changes I've made to the behaviour are cases where NaN/Infinity (/maybe subnormals) crop up.

NaN/infinity aren’t always invalid in shader land. There is a trick used to discard vertices by setting their position to NaN for example. I’d be careful about deciding to explicitly check and handle NaNs unless it is very clear that a NaN at that point is invalid and has to be handled.

The vast majority of the code doesn't do any floating point handling, and is just moving data around in hilariously evil ways. The parts that do floating point operations, I think I've done an exact translation.

Right, totally down with making how the data is moved around be safe and rusty. And if indeed the float operations are an exact translation (including allowing NaNs or whatever, I guess, until proven that handling them differently is the correct thing to do) then this sounds great. Just saying this for the purpose of alignment. :)

[DJMcNab]

As an example, as far as I can tell, if there's a single vertex with infinite position, this line means that none of the vertices get merged.
However, identical vertices being merged is relied upon for correctness.

So, assuming that all positions are finite, what next?

Following through the code, eventually you get to calling m_setTSpace with NaN or Infinity vectors, which breaks the contract of that method.

This is in C api rules, which best as I can tell is that any input which gets garbage out must be an illegal input.

The only mention of NaN handling in mikktspace I can find is here, which prevents an infinite loop when they occur. In the original paper, the word NaN does not appear, not does Infinity except in a single mention for (as best as I can tell) something unrelated.

Additionally, as best as I can tell from a quick check, weird values are forbidden in GLTF.

Basically, I'm pretty confident that panicking in the prescense of NaN/infinity is fine.

[DJMcNab]

I'm not likely to drive this forward, so I'll defer to #9050