The Big Refactor (#7)

* The Big Refactor

* Amend copyright notice

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

* Check features & docs in CI

* Revert unintended change to documentation

* Add benchmarks

* Better documentation

---------

Co-authored-by: Alice Cecile <[email protected]>

Author: bushrat011899

.github/workflows/ci.yml

@@ -34,6 +34,10 @@ jobs:
         run: |
           cargo clippy -- -D warnings
 
+      - name: Run clippy (all features)
+        run: |
+          cargo clippy --all-features -- -D warnings
+
   check:
     name: Check
     runs-on: ubuntu-latest
@@ -59,3 +63,33 @@ jobs:
       - name: Test
         run: |
           cargo test
+
+  doc:
+    name: Documentation
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      - uses: dtolnay/rust-toolchain@stable
+
+      - name: Check Docs
+        run: |
+          RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items
+
+      - name: Check Docs (all features)
+        run: |
+          RUSTDOCFLAGS="-D warnings" cargo doc --all-features --document-private-items
+
+  benches:
+    name: Benchmarks
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      - uses: dtolnay/rust-toolchain@stable
+
+      - name: Check Benchmarks
+        run: |
+          cargo bench --no-run

Cargo.toml

@@ -3,9 +3,12 @@ name = "bevy_mikktspace"
 version = "0.17.0-dev"
 edition = "2024"
 authors = [
+    "Morten S. Mikkelsen <[email protected]>",
     "Benjamin Wasty <[email protected]>",
     "David Harvey-Macaulay <[email protected]>",
     "Layl Bongers <[email protected]>",
+    "Atlas Dostal <[email protected]>",
+    "Zac Harrold <[email protected]>",
 ]
 description = "Safe rust implementation of Mikkelsen tangent space algorithm"
 documentation = "https://docs.rs/bevy_mikktspace"
@@ -15,19 +18,46 @@ license = "Zlib AND (MIT OR Apache-2.0)"
 keywords = ["bevy", "3D", "graphics", "algorithm", "tangent"]
 rust-version = "1.85.0"
 
-exclude = ["/data", "/generator"]
+exclude = ["/data", "/fuzz"]
 
 [dependencies]
-bitflags = "2.3"
+# This space is intentionally left blank
+
+[features]
+default = ["std"]
+std = []
+
+# 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 = []
 
 [dev-dependencies]
 mikktspace-sys = "0.1.1"
 wavefront_obj = "11.0.0"
-glam = "0.30.4"
+libm = "0.2.15"
+criterion = "0.6.0"
+
+[lints.rust.unexpected_cfgs]
+level = "warn"
+check-cfg = ['cfg(mikktspace_rs_more_assertions)']
 
 [[example]]
 name = "cube_tangents"
 
+[[bench]]
+name = "plane"
+harness = false
+
 [package.metadata.docs.rs]
 rustdoc-args = ["-Zunstable-options", "--generate-link-to-definition"]
 all-features = true

benches/plane.rs

@@ -0,0 +1,112 @@
+use criterion::{Criterion, criterion_group, criterion_main};
+use std::hint::black_box;
+
+/// A unit plane with corners at `(0, 0)` and `(1, 1)` in the XY plane.
+/// `N` is the number of quads used to subdivide this plane.
+struct SubdividedPlane<const N: usize>;
+
+impl<const N: usize> bevy_mikktspace::Geometry for SubdividedPlane<N> {
+    fn num_faces(&self) -> usize {
+        N * N
+    }
+
+    fn num_vertices_of_face(&self, _face: usize) -> usize {
+        4
+    }
+
+    fn position(&self, face: usize, vert: usize) -> [f32; 3] {
+        let f_y = face % N;
+        let f_x = face / N;
+
+        let (v_x, v_y) = match vert {
+            0 => (f_x, f_y),
+            1 => (f_x, f_y + 1),
+            2 => (f_x + 1, f_y + 1),
+            3 => (f_x + 1, f_y),
+            _ => panic!(),
+        };
+
+        [
+            v_x as f32 / (N as f32 + 1.),
+            v_y as f32 / (N as f32 + 1.),
+            0.,
+        ]
+    }
+
+    fn normal(&self, _face: usize, _vert: usize) -> [f32; 3] {
+        [0., 0., 1.]
+    }
+
+    fn tex_coord(&self, face: usize, vert: usize) -> [f32; 2] {
+        let f_y = face % N;
+        let f_x = face / N;
+
+        let (v_x, v_y) = match vert {
+            0 => (f_x, f_y),
+            1 => (f_x, f_y + 1),
+            2 => (f_x + 1, f_y + 1),
+            3 => (f_x + 1, f_y),
+            _ => panic!(),
+        };
+
+        [v_x as f32 / (N as f32 + 1.), v_y as f32 / (N as f32 + 1.)]
+    }
+
+    fn set_tangent(
+        &mut self,
+        tangent_space: Option<bevy_mikktspace::TangentSpace>,
+        _face: usize,
+        _vert: usize,
+    ) {
+        let _ = black_box(tangent_space);
+    }
+}
+
+impl<const N: usize> mikktspace_sys::MikkTSpaceInterface for SubdividedPlane<N> {
+    fn get_num_faces(&self) -> usize {
+        <Self as bevy_mikktspace::Geometry>::num_faces(&self)
+    }
+
+    fn get_num_vertices_of_face(&self, face: usize) -> usize {
+        <Self as bevy_mikktspace::Geometry>::num_vertices_of_face(&self, face)
+    }
+
+    fn get_position(&self, face: usize, vert: usize) -> [f32; 3] {
+        <Self as bevy_mikktspace::Geometry>::position(&self, face, vert)
+    }
+
+    fn get_normal(&self, face: usize, vert: usize) -> [f32; 3] {
+        <Self as bevy_mikktspace::Geometry>::normal(&self, face, vert)
+    }
+
+    fn get_tex_coord(&self, face: usize, vert: usize) -> [f32; 2] {
+        <Self as bevy_mikktspace::Geometry>::tex_coord(&self, face, vert)
+    }
+
+    fn set_tspace_basic(&mut self, tangent: [f32; 3], _sign: f32, _face: usize, _vert: usize) {
+        let _ = black_box(tangent);
+    }
+}
+
+fn criterion_benchmark(c: &mut Criterion) {
+    // Bevy's implementation
+    c.bench_function("bevy", |b| {
+        b.iter(|| {
+            let _ = black_box(bevy_mikktspace::generate_tangents(black_box(
+                &mut SubdividedPlane::<32>,
+            )));
+        })
+    });
+
+    // Original C implementation over FFI
+    c.bench_function("original", |b| {
+        b.iter(|| {
+            let _ = black_box(mikktspace_sys::gen_tang_space_default(black_box(
+                &mut SubdividedPlane::<32>,
+            )));
+        })
+    });
+}
+
+criterion_group!(benches, criterion_benchmark);
+criterion_main!(benches);

data/LICENSE.txt

@@ -0,0 +1,121 @@
+Creative Commons Legal Code
+
+CC0 1.0 Universal
+
+    CREATIVE COMMONS CORPORATION IS NOT A LAW FIRM AND DOES NOT PROVIDE
+    LEGAL SERVICES. DISTRIBUTION OF THIS DOCUMENT DOES NOT CREATE AN
+    ATTORNEY-CLIENT RELATIONSHIP. CREATIVE COMMONS PROVIDES THIS
+    INFORMATION ON AN "AS-IS" BASIS. CREATIVE COMMONS MAKES NO WARRANTIES
+    REGARDING THE USE OF THIS DOCUMENT OR THE INFORMATION OR WORKS
+    PROVIDED HEREUNDER, AND DISCLAIMS LIABILITY FOR DAMAGES RESULTING FROM
+    THE USE OF THIS DOCUMENT OR THE INFORMATION OR WORKS PROVIDED
+    HEREUNDER.
+
+Statement of Purpose
+
+The laws of most jurisdictions throughout the world automatically confer
+exclusive Copyright and Related Rights (defined below) upon the creator
+and subsequent owner(s) (each and all, an "owner") of an original work of
+authorship and/or a database (each, a "Work").
+
+Certain owners wish to permanently relinquish those rights to a Work for
+the purpose of contributing to a commons of creative, cultural and
+scientific works ("Commons") that the public can reliably and without fear
+of later claims of infringement build upon, modify, incorporate in other
+works, reuse and redistribute as freely as possible in any form whatsoever
+and for any purposes, including without limitation commercial purposes.
+These owners may contribute to the Commons to promote the ideal of a free
+culture and the further production of creative, cultural and scientific
+works, or to gain reputation or greater distribution for their Work in
+part through the use and efforts of others.
+
+For these and/or other purposes and motivations, and without any
+expectation of additional consideration or compensation, the person
+associating CC0 with a Work (the "Affirmer"), to the extent that he or she
+is an owner of Copyright and Related Rights in the Work, voluntarily
+elects to apply CC0 to the Work and publicly distribute the Work under its
+terms, with knowledge of his or her Copyright and Related Rights in the
+Work and the meaning and intended legal effect of CC0 on those rights.
+
+1. Copyright and Related Rights. A Work made available under CC0 may be
+protected by copyright and related or neighboring rights ("Copyright and
+Related Rights"). Copyright and Related Rights include, but are not
+limited to, the following:
+
+  i. the right to reproduce, adapt, distribute, perform, display,
+     communicate, and translate a Work;
+ ii. moral rights retained by the original author(s) and/or performer(s);
+iii. publicity and privacy rights pertaining to a person's image or
+     likeness depicted in a Work;
+ iv. rights protecting against unfair competition in regards to a Work,
+     subject to the limitations in paragraph 4(a), below;
+  v. rights protecting the extraction, dissemination, use and reuse of data
+     in a Work;
+ vi. database rights (such as those arising under Directive 96/9/EC of the
+     European Parliament and of the Council of 11 March 1996 on the legal
+     protection of databases, and under any national implementation
+     thereof, including any amended or successor version of such
+     directive); and
+vii. other similar, equivalent or corresponding rights throughout the
+     world based on applicable law or treaty, and any national
+     implementations thereof.
+
+2. Waiver. To the greatest extent permitted by, but not in contravention
+of, applicable law, Affirmer hereby overtly, fully, permanently,
+irrevocably and unconditionally waives, abandons, and surrenders all of
+Affirmer's Copyright and Related Rights and associated claims and causes
+of action, whether now known or unknown (including existing as well as
+future claims and causes of action), in the Work (i) in all territories
+worldwide, (ii) for the maximum duration provided by applicable law or
+treaty (including future time extensions), (iii) in any current or future
+medium and for any number of copies, and (iv) for any purpose whatsoever,
+including without limitation commercial, advertising or promotional
+purposes (the "Waiver"). Affirmer makes the Waiver for the benefit of each
+member of the public at large and to the detriment of Affirmer's heirs and
+successors, fully intending that such Waiver shall not be subject to
+revocation, rescission, cancellation, termination, or any other legal or
+equitable action to disrupt the quiet enjoyment of the Work by the public
+as contemplated by Affirmer's express Statement of Purpose.
+
+3. Public License Fallback. Should any part of the Waiver for any reason
+be judged legally invalid or ineffective under applicable law, then the
+Waiver shall be preserved to the maximum extent permitted taking into
+account Affirmer's express Statement of Purpose. In addition, to the
+extent the Waiver is so judged Affirmer hereby grants to each affected
+person a royalty-free, non transferable, non sublicensable, non exclusive,
+irrevocable and unconditional license to exercise Affirmer's Copyright and
+Related Rights in the Work (i) in all territories worldwide, (ii) for the
+maximum duration provided by applicable law or treaty (including future
+time extensions), (iii) in any current or future medium and for any number
+of copies, and (iv) for any purpose whatsoever, including without
+limitation commercial, advertising or promotional purposes (the
+"License"). The License shall be deemed effective as of the date CC0 was
+applied by Affirmer to the Work. Should any part of the License for any
+reason be judged legally invalid or ineffective under applicable law, such
+partial invalidity or ineffectiveness shall not invalidate the remainder
+of the License, and in such case Affirmer hereby affirms that he or she
+will not (i) exercise any of his or her remaining Copyright and Related
+Rights in the Work or (ii) assert any associated claims and causes of
+action with respect to the Work, in either case contrary to Affirmer's
+express Statement of Purpose.
+
+4. Limitations and Disclaimers.
+
+ a. No trademark or patent rights held by Affirmer are waived, abandoned,
+    surrendered, licensed or otherwise affected by this document.
+ b. Affirmer offers the Work as-is and makes no representations or
+    warranties of any kind concerning the Work, express, implied,
+    statutory or otherwise, including without limitation warranties of
+    title, merchantability, fitness for a particular purpose, non
+    infringement, or the absence of latent or other defects, accuracy, or
+    the present or absence of errors, whether or not discoverable, all to
+    the greatest extent permissible under applicable law.
+ c. Affirmer disclaims responsibility for clearing rights of other persons
+    that may apply to the Work or any use thereof, including without
+    limitation any person's Copyright and Related Rights in the Work.
+    Further, Affirmer disclaims responsibility for obtaining any necessary
+    consents, permissions or other rights required for any use of the
+    Work.
+ d. Affirmer understands and acknowledges that Creative Commons is not a
+    party to this document and has no duty or obligation with respect to
+    this CC0 or use of the Work.