Skip to content

Conversation

bushrat011899
Copy link
Contributor

Objective

Currently, testing relies of a C project to generate reference data and requires storing binary blobs in-repo to compare against. This is not ideal from a security perspective, and makes contributing harder for those unfamiliar with C-styled projects.

Solution

  • Added mikktspace-sys, a popular binding to the original C implementation of mikktspace, as a dev-dependency.
  • Added a shim type, Shadow, which wraps an implementation of Geometry and will internally compare results to mikktspace-sys, panicking on difference. This is only enabled when #[cfg(test)] is enabled, so this will never affect end-users.
  • Removed the binary blobs and C project to generate them.
  • Refactored the regression tests to directly load OBJ files. Since the reference data is computed live within the test, there is no need to store expected results.

Notes

  • By putting the assertion directly inside generate_tangents, we can ensure that all tests will compare their results to the C implementation. This allows for future fuzz-testing to also act as regression testing.
  • While this PR is independent of Simplify Implementation #3, I would recommend merging this one first. Simplified and more approachable testing now will make the refactoring PR easier to assess on its own merits.

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing that maybe suzanne shouldn't be included here, as the license may not be permissive. (I know its already present in the repo, thats an oversight.)

Looks good other than that

@bushrat011899
Copy link
Contributor Author

I'm realizing that maybe suzanne shouldn't be included here, as the license may not be permissive. (I know its already present in the repo, thats an oversight.)

Fair. The general consensus among the Blender community appears to be they consider Suzanne to be program output and not in of itself subject to GPL, but better safe than sorry. I'll update this PR to just remove the Suzanne files entirely, but I recommend another PR add back some new (appropriately licensed) models.

@alice-i-cecile
Copy link
Member

Agree with cutting Suzanne, and also agree with adding back new models.

I like the idea of using mikktspace-sys much better as well.

@bushrat011899
Copy link
Contributor Author

Resolved the merge conflicts and removed the Suzanne-derived models. I'll leave finding appropriate replacements for a follow-up.

@alice-i-cecile alice-i-cecile requested a review from atlv24 June 30, 2025 23:02
@alice-i-cecile alice-i-cecile merged commit 87a54b6 into bevyengine:main Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants