The Big Refactor #7

bushrat011899 · 2025-07-16T08:05:44Z

Objective

It has been discovered through fuzzing that the current bevy_mikktspace is no longer byte-compatible with the original C implementation. Using git bisect, I determined that the discrepancy was introduced before the transition to safe Rust. This makes diagnosing and resolving the issue almost impossible.

So I started again from scratch.

Using c2rust, cargo-fuzz, test data produced by @atlv24, and the original safe translation by @LaylBongers, I recreated a safe port of mikktspace in safe Rust here.

This PR moves the changes there to bevy_mikktspace.

Solution

Added fuzzing using cargo-fuzz with built-in comparison to mikktspace-sys (the original C implementation with Rust bindings).
Added test data from atlv24 to better catch known edge cases.
Adjusted Geometry API to avoid mutually exclusive trait methods for setting tangent values.
Migrated no_std support to directly receive the two missing float methods (sqrt and acos) rather than a higher level Vec3 implementation.
Added an std feature to provide a byte-compatible default. This is motivated by a desire to make this a viable replacement for mikktspace-sys for non-Bevy use-cases.
Massively refactored the original C implementation to more idiomatic Rust. This includes better utilization of iterator techniques rather than preallocated buffers and indexing.
Added documentation explaining how and why certain code is employed, especially where a more idiomatic option isn't used.
Added features to provide more "correct" results (fixing original C bugs) at the expense of byte-compatibility.
Supersedes add test data #5

Notes

This is a complete replacement of the original code, so it may be easier to just check out and read rather than reviewing online.

bushrat011899

Just some comments to assist reviewers.

bushrat011899 · 2025-07-16T08:19:04Z

Cargo.toml


 [dependencies]
-bitflags = "2.3"


Because of alignment, there was actually no space savings in the Rust implementation using bitflags over the far simpler "just use 4 bools".

bushrat011899 · 2025-07-16T08:19:55Z

Cargo.toml

+# The below features will cause the resulting values to differ from the original
+# C implementation.
+
+# Corrects a sorting bug in the original C implementation.
+# See https://github.com/mmikk/MikkTSpace/issues/5 for details
+corrected-edge-sorting = []
+
+# Uses a BTreeMap to weld vertices which is guaranteed to use the smallest vertex
+# indices.
+# This handles `NaN` values differently to the C implementation, so will produce
+# different results for poor geometry.
+# For typical geometry, this should produce identical results.
+corrected-vertex-welding = []


These features might be worth prefixing with "unstable" so we can hit 1.0.

bushrat011899 · 2025-07-16T08:21:47Z

examples/cube_tangents.rs

@@ -23,31 +21,12 @@ struct Mesh {
    vertices: Vec<Vertex>,
 }

-struct GlamSpace;
-
-impl bevy_mikktspace::VectorSpace for GlamSpace {


I know this was something I introduced only a few weeks ago, but having spent all that time neck deep in the unsafe C version, I think I prefer the simplicity of just giving the 2 missing float methods.

Plus, an std feature on by default improves the typical user experience.

bushrat011899 · 2025-07-16T08:23:01Z

examples/cube_tangents.rs

-    fn set_tangent_encoded(&mut self, tangent: [f32; 4], face: usize, vert: usize) {
+    fn set_tangent(
+        &mut self,
+        tangent: Option<bevy_mikktspace::TangentSpace>,


This change allows both set_tangent methods to be combined, avoiding the somewhat ugly "implement one of these" pattern that was a holdover from the original C implementation.

Also note I've explicitly replaced this with an Option. The original C implementation will silently return a default value under certain circumstances, which I think is something worth highlighting to the user, as they may have a more sensible default. TangentSpace implements Default, so users can get the exact behaviour anyway.

bushrat011899 · 2025-07-16T08:25:03Z

examples/cube_tangents.rs

@@ -280,7 +264,7 @@ fn make_cube() -> Mesh {

 fn main() {
    let mut cube = make_cube();
-    bevy_mikktspace::generate_tangents(&mut cube);
+    let _ = bevy_mikktspace::generate_tangents(&mut cube);


We don't return bool in Rust to indicate success! We have Result!

bushrat011899 · 2025-07-16T08:37:36Z

tests/sample_data.rs

+generate_tests! {
+    cube: "../data/cube.obj",
+    crangeract: "../data/crangeract.obj",
+    doomcone_smooth: "../data/doomcone_smooth.obj",
+    doomcone: "../data/doomcone.obj",
+    obliterated: "../data/obliterated.obj",
+    rancid_geometry: "../data/rancid_geometry.obj",
+    tangeract: "../data/tangeract.obj",
+}


For all of these tests we directly compare to the C implementation to ensure identical results.

bushrat011899 · 2025-07-16T08:38:25Z

src/mikktspace.rs

+mod face_vertex;
+#[cfg_attr(
+    not(feature = "corrected-edge-sorting"),
+    path = "mikktspace/quick_sort_legacy.rs"
+)]
+mod quick_sort;
+mod raw_tangent_space;
+#[cfg_attr(
+    not(feature = "corrected-vertex-welding"),
+    path = "mikktspace/weld_vertices_legacy.rs"
+)]
+mod weld_vertices;


I've split out some of this file into a couple of supporting modules, just to help with clarity and feature gating.

bushrat011899 · 2025-07-16T08:40:38Z

src/mikktspace.rs

+///
+/// This is separated from [`generate_tangent_space_and_write`] to highlight this
+/// step does not require mutable access to the provided [context](MikkTSpaceInterface).
+fn generate_tangent_space<I: Geometry<O>, O: Ops>(


Compared to the original C implementation, I've reordered some operations to simplify. For example, the vertex list isn't actually used until much later in the algorithm, but it's generated early and must be amended as the triangle list is modified to keep them in sync. Instead, I generate the list towards the end and skip all the intermediate busy work.

bushrat011899 · 2025-07-16T08:41:43Z

src/mikktspace.rs

+) -> Result<Vec<Option<TangentSpace<O>>>, GenerateTangentSpaceError> {
+    // This set of operations can be done on-line.
+    // That is, they do not require an intermediatory buffer.
+    let mut triangle_info_list = generate_triangle_info_list(context, faces_total);


I've moved most of the manipulation of triangle information into this method, as the original implementation iterates over the list many times, when it could instead perform most of the tasks in one go. With Rust iterators that's very trivial.

bushrat011899 · 2025-07-16T08:42:46Z

src/mikktspace.rs

+struct TriangleInfo<O: Ops> {
+    /// Stores the index of each neighboring triangle to this one, if any.
+    /// Indices correspond to _edges_ of this triangle, rather than _vertices_.
+    face_neighbors: [Option<usize>; 3],


usize increases the size of this type a lot, but it's idiomatic Rust so I'd prefer it over u32 indexing...

NonMaxUsize? will this always be 0..3

Ah so this is a bit of bad documentation on my part. The Option<usize> value is an index into the big list of all triangles. The edge value is bounded to 0..3, but is the index into the face_neighbors array itself, not the value stored in each slot. I've updated the documentation to better explain that.

As for NonMaxUsize, I agree, but I'd rather not. The goal is to get this to 1.0 and never worry about it again, whereas the nonmax crate is still pre-1.0. Technically it shouldn't matter, as it wouldn't be a part of the public API, but it'd still potentially cause duplicated dependencies in the future, etc.

src/math.rs

alice-i-cecile · 2025-07-16T19:33:18Z

src/mikktspace/face_vertex.rs

@@ -0,0 +1,33 @@
+/// New-type index for a particular vertex on a particular face.


Documenting the winding order here would be helpful.

The way mikktspace works should be independent of the winding order:

This is done by performing an internal welding step and subsequently an
order-independent evaluation of tangent space for meshes consisting of
triangles and quads.
This means faces can be received in any order and the same is true for the
order of vertices of each face.
The generated result will not be affected by such reordering.

But I'll leave this comment unresolved just in case someone with better rendering experience would like to weigh in!

src/mikktspace.rs

alice-i-cecile

Non-malicious, completely safe, generally very clear. I am very much leaning on tests for correctness.

I have a few small nits, and I would like to clarify the copyright notices before we merge this. I know the standard style is "just dump it at the top of the file", but I always find that very unhelpful and unclear about why the notice exists at all.

Co-Authored-By: Alice Cecile <[email protected]>

bushrat011899 · 2025-07-16T23:57:41Z

Added a benchmark which compares bevy_mikktspace against mikktspace-sys on a subdivided plane (something simple and can be generated on the fly to allow huge models if desired). For a 32x32 quad, both implementations complete the benchmark in around 1.9ms (plus or minus 0.1ms). Changing the number of subdivisions drastically changes the runtime, but the relative performance stays largely the same; within percentage points of each other.

The Big Refactor

e5e4126

bushrat011899 commented Jul 16, 2025

View reviewed changes

alice-i-cecile reviewed Jul 16, 2025

View reviewed changes

src/math.rs Show resolved Hide resolved

alice-i-cecile reviewed Jul 16, 2025

View reviewed changes

src/mikktspace.rs Show resolved Hide resolved

alice-i-cecile reviewed Jul 16, 2025

View reviewed changes

bushrat011899 and others added 4 commits July 17, 2025 08:53

Amend copyright notice

69147ea

Co-Authored-By: Alice Cecile <[email protected]>

Check features & docs in CI

924b6ec

Revert unintended change to documentation

7b0349a

Add benchmarks

4844abe

alice-i-cecile approved these changes Jul 17, 2025

View reviewed changes

alice-i-cecile requested review from mockersf and atlv24 July 17, 2025 19:41

Better documentation

e17e98f

		@@ -0,0 +1,33 @@
		/// New-type index for a particular vertex on a particular face.

The Big Refactor #7

Are you sure you want to change the base?

The Big Refactor #7

Uh oh!

Conversation

bushrat011899 commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Notes

Uh oh!

bushrat011899 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

bushrat011899 commented Jul 16, 2025

Uh oh!

Uh oh!

bushrat011899 commented Jul 16, 2025 •

edited

Loading