Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Generate vertex tangents using mikktspace #3872

Closed
wants to merge 106 commits into from

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Feb 5, 2022

Objective

Models can be produced that do not have vertex tangents but do have normal map textures. The tangents can be generated. There is a way that the vertex tangents can be generated to be exactly invertible to avoid introducing error when recreating the normals in the fragment shader.

Solution

  • After attempts to get https://github.com/gltf-rs/mikktspace to integrate simple glam changes and version bumps, and releases of that crate taking weeks / not being made (no offense intended to the authors/maintainers, bevy just has its own timelines and needs to take care of) it was decided to fork that repository. The following steps were taken:
    • mikktspace was forked to https://github.com/bevyengine/mikktspace in order to preserve the repository's history in case the original is ever taken down
    • The README in that repo was edited to add a note stating from where the repository was forked and explaining why
    • The repo was locked for changes as its only purpose is historical
    • The repo was integrated into the bevy repo using git subtree add --prefix crates/bevy_mikktspace git@github.com:bevyengine/mikktspace.git master
    • In bevy_mikktspace:
      • The travis configuration was removed
      • cargo fmt was run
      • The Cargo.toml was conformed to bevy's (just adding bevy to the keywords, changing the homepage and repository, changing the version to 0.7.0-dev - importantly the license is exactly the same)
      • Remove the features, remove nalgebra entirely, only use glam, suppress clippy.
        • This was necessary because our CI runs clippy with --all-features and the nalgebra and glam features are mutually exclusive, plus I don't want to modify this highly numerically-sensitive code just to appease clippy and diverge even more from upstream.
  • Rebase generate tangents if they're missing and a normal map exists #1795
    • @jakobhellermann said it was fine to copy and paste but it ended up being almost exactly the same with just a couple of adjustments when validating correctness so I decided to actually rebase it and then build on top of it.
  • Use the exact same fragment shader code to ensure correct normal mapping.
  • Tested with both https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentMirrorTest which has vertex tangents and https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentTest which requires vertex tangent generation

bwasty and others added 30 commits June 30, 2017 15:44
Former-commit-id: 5d11cb5b8100a169c105405e75d58399caf6f57d
Former-commit-id: 0289b4d33cb63b4a89994291bed38e5a4e604712
Former-commit-id: 7bd77bfa85539326f688ead84edaf086c5c88985
Former-commit-id: 76f4a864fa3b1fbde9d5ac2eab7c5d0639d147ef
Former-commit-id: fb3b6b7f4a340a1e1afe156697cc60d7d7b34117
Former-commit-id: 909221683bc0bf7a15458b710e98c4e1565b71fb
Former-commit-id: ba8b4c7769097e5b50fc3e12a85fc71d06b5a2ae
Former-commit-id: 89f4dc6126d72e931954bdedb319ee44376656cc
Add bindings to C reference implementation

Former-commit-id: 8964a4bb729250aedd7f53bc4d6af9adff3c9ef1
Former-commit-id: a9a4929c42f353430eaf80f5c49526a4b7111b4b
Former-commit-id: 1d6faf7c33382998c671c6ecc0d8d9f963660522
Former-commit-id: b63a3688c8593f06f2a7870308def5c13845003e
Former-commit-id: 95ceac69a9a32ab8c9eea29045efd02a068c6db0
Set libmikktspace version to crate version

Former-commit-id: 5b23b1c6ebd7f4e9b3537d387cbafeb594ae2820
Add equivalent C generate example

Former-commit-id: f881f983d4cfebdc20ff46aba0f061c17c26117e
Former-commit-id: 426d7fb539a3c54c70d0cf376e4f3a058bfebd82
Former-commit-id: d78472e2d7724f28ec29bb1a1c1727eca27c11c1
Former-commit-id: 8654d95d71ffefc456d185362c4b8e139b35cb52
Former-commit-id: 24b6ba657455a761ccfeee98bf26d3da663dfc72
Former-commit-id: 54f656f9532c4a62be935438ba4bdab189d860b5
Former-commit-id: 260211540cf844e558139f1157a5c6f42587dd67
Former-commit-id: 4cb2812da7e4230d7fe0bc6620afa697cb6b23ee
This closes issue #5


Former-commit-id: 8fe70edc1d9b9b3fb6899307e5ef241ebb1a2f0d
Former-commit-id: 3700dcc0bc70d6df135447116b57c3117ae23a13
@superdump
Copy link
Contributor Author

I've updated the PR description but I have now done this:

  • After attempts to get https://github.com/gltf-rs/mikktspace to integrate simple glam changes and version bumps, and releases of that crate taking weeks / not being made (no offense intended to the authors/maintainers, bevy just has its own timelines and needs to take care of) it was decided to fork that repository. The following steps were taken:
    • mikktspace was forked to https://github.com/bevyengine/mikktspace in order to preserve the repository's history in case the original is ever taken down
    • The README in that repo was edited to add a note stating from where the repository was forked and explaining why
    • The repo was locked for changes as its only purpose is historical
    • The repo was integrated into the bevy repo using git subtree add --prefix crates/bevy_mikktspace git@github.com:bevyengine/mikktspace.git master
    • In bevy_mikktspace:
      • The travis configuration was removed
      • cargo fmt was run
      • The Cargo.toml was conformed to bevy's (just adding bevy to the keywords, changing the homepage and repository, changing the version to 0.7.0-dev - importantly the license is exactly the same)
      • Remove the features, remove nalgebra entirely, only use glam, suppress clippy.
        • This was necessary because our CI runs clippy with --all-features and the nalgebra and glam features are mutually exclusive, plus I don't want to modify this highly numerically-sensitive code just to appease clippy and diverge even more from upstream

@superdump superdump force-pushed the generate-tangents branch 2 times, most recently from f12f2b5 to 04ac134 Compare May 2, 2022 11:19
@cart cart modified the milestones: Bevy 0.7, Bevy 0.8 May 3, 2022
crates/bevy_mikktspace/Cargo.toml Outdated Show resolved Hide resolved
@@ -309,6 +304,23 @@ async fn load_gltf<'a, 'b>(
}
}

if let Some(vertex_attribute) = reader
Copy link
Member

Choose a reason for hiding this comment

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

For future reviewers, this reordering is done to make sure that all the data has been added to the mesh before bevy_mikktspace is run.

crates/bevy_gltf/src/loader.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,35 @@
# mikktspace

This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It will be vendored in the bevy repository itself as crates/bevy_mikktspace.
Copy link
Member

Choose a reason for hiding this comment

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

This readme should be updated further imo.

Copy link
Member

Choose a reason for hiding this comment

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

With what info specifically?

Copy link
Member

@DJMcNab DJMcNab May 28, 2022

Choose a reason for hiding this comment

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

It should follow the readme pattern of other bevy crates.
For example, the Travis status badge is wrong, the name in the header is incorrect, the crates.io link is wrong (by the way, whilst we're here we should reserve bevy_mikktspace on crates.io)

Also the license section could be updated to mention the zlib part. But that might not be needed.

Only minor concerns really, but worth fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the main heading to bevy_mikktspace and removed the two lines for travis and crates.io.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DJMcNab are the changes acceptable?

Copy link
Member

@DJMcNab DJMcNab May 31, 2022

Choose a reason for hiding this comment

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

Suggested change
This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It will be vendored in the bevy repository itself as crates/bevy_mikktspace.
This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It is vendored in the bevy repository itself as [crates/bevy_mikktspace](https://github.com/bevyengine/bevy/tree/main/crates/bevy_mikktspace)

But otherwise I'm reasonably happy. We should consider having the standard badges for the bevy crates. However, I suspect we'll want to update those at some point - https://github.com/EmbarkStudios/opensource-template would be a neat header style to follow

/// Returns `false` if the geometry is unsuitable for tangent generation including,
/// but not limited to, lack of vertices.
pub fn generate_tangents<I: Geometry>(geometry: &mut I) -> bool {
unsafe { generated::genTangSpace(geometry, 180.0) }
Copy link
Member

Choose a reason for hiding this comment

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

What's our stance on making this safer? Would we do that in place or upstream it?

My inclination would be to do them upstream, but that would be potentially slow to land, and annoying to merge back into here.
I don't want the path of least resistance to be just not fixing the unsafety, because of uncertainty about where to land it.

Copy link
Member

Choose a reason for hiding this comment

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

Imo bevy_mikktspace should be our new source of truth and we should try to improve safety if we can. We are "hard forking". If the original author is interested in upstreaming our changes, they can be proactive about it.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I think this is good to go, once comments are resolved (including @DJMcNab's).

crates/bevy_mikktspace/.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Handful of tiny nits, nothing blocking.

I also did one quick reading pass over generated.rs, just to make sure it looked mostly right. Don't want you sneaking a crypto miner into our code :P !

{
mesh.insert_attribute(Mesh::ATTRIBUTE_TANGENT, vertex_attribute);
} else if mesh.attribute(Mesh::ATTRIBUTE_NORMAL).is_some()
&& primitive.material().normal_texture().is_some()
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how meshes work here, but why do we need to know that the material has a normal_texture? It doesn't look like the material is passed into generate_tangents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The shader code needs the normal vertex attribute, a normal map texture, and tangent vertex attribute in order to apply normal mapping. The normal map texture is in tangent space, the combination of the vertex normal and vertex tangent can then be used to transform the tangent space normal to a world space normal. So, if we don't have both a normal map texture and normal attribute, there is no need to generate tangents.

Copy link
Member

Choose a reason for hiding this comment

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

I recognise all of those words.

I'll defer to your judgement here :)

glam = "0.20.0"

[[example]]
name = "generate"
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we should have this tested end-to-end somewhere higher up the tree - or at least a visual example. I can't say that we should be blocked on that, and I don't understand what tangents are enough to say what form that should take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. There are Khronos glTF example models for testing that could be used and they are freely-licensed. I don't like adding binary blobs into the repo though. @cart what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

There are already regression tests elsewhere in the project that will be run via cargo test. I'm cool with leaving it at that for now.

crates/bevy_mikktspace/LICENSE-APACHE Outdated Show resolved Hide resolved

## License agreement

Licensed under either of
Copy link
Member

Choose a reason for hiding this comment

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

We should probably mention that there are sections licensed under the original zlib license of upstream mikktspace here, but contributions don't have that license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. See if you think the approach is ok. It's a little tricky to cover this AND either of these in a clear way.

crates/bevy_mikktspace/Cargo.toml Outdated Show resolved Hide resolved
name = "bevy_mikktspace"
version = "0.8.0-dev"
edition = "2021"
authors = ["Benjamin Wasty <benny.wasty@gmail.com>", "David Harvey-Macaulay <alteous@outlook.com>", "Layl Bongers <LaylConway@users.noreply.github.com>"]
Copy link
Member

Choose a reason for hiding this comment

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

We could consider removing the authors field here. I don't want to make that call one way or another.

One reason we may want to do so is that it's no longer recommended practise to have it. OTOH I don't think it hurts anything to leave it here.

Copy link
Member

Choose a reason for hiding this comment

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

If we ever meaningfully diverge from the current impl, I'm more comfortable considering removing this. Until then, I'd prefer to leave it in as a form of credit.

@@ -0,0 +1,35 @@
# mikktspace

This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It will be vendored in the bevy repository itself as crates/bevy_mikktspace.
Copy link
Member

@DJMcNab DJMcNab May 31, 2022

Choose a reason for hiding this comment

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

Suggested change
This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It will be vendored in the bevy repository itself as crates/bevy_mikktspace.
This is a fork of [https://github.com/gltf-rs/mikktspace](https://github.com/gltf-rs/mikktspace), which in turn is a port of the Mikkelsen Tangent Space Algorithm reference implementation to Rust. It has been forked for use in the bevy game engine to be able to update maths crate dependencies in lock-step with bevy releases. It is vendored in the bevy repository itself as [crates/bevy_mikktspace](https://github.com/bevyengine/bevy/tree/main/crates/bevy_mikktspace)

But otherwise I'm reasonably happy. We should consider having the standard badges for the bevy crates. However, I suspect we'll want to update those at some point - https://github.com/EmbarkStudios/opensource-template would be a neat header style to follow

@@ -0,0 +1,259 @@
#![allow(clippy::bool_assert_comparison, clippy::useless_conversion)]
Copy link
Member

Choose a reason for hiding this comment

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

Can we easily fix these? We should at least have comments explaining why they're needed if not, and have them be on the relevant statements/blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CI target is failing so we will need to decide whether we suppress the clippy warnings for the entire file, or go through and fix them all. I would rather not fix them in this initial commit because I don't want to touch the code any more than necessary. Any changes we make would then be bisectable back to this, which is better for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cart ^

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to suppress these first and then follow up later with fixes if we see the need. That way we have the original state in our history.

Copy link
Member

Choose a reason for hiding this comment

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

Just suppressed the remaining lints (we recently merged some new ones).

@cart
Copy link
Member

cart commented May 31, 2022

bors r+

bors bot pushed a commit that referenced this pull request May 31, 2022
# Objective

Models can be produced that do not have vertex tangents but do have normal map textures. The tangents can be generated. There is a way that the vertex tangents can be generated to be exactly invertible to avoid introducing error when recreating the normals in the fragment shader.

## Solution

- After attempts to get https://github.com/gltf-rs/mikktspace to integrate simple glam changes and version bumps, and releases of that crate taking weeks / not being made (no offense intended to the authors/maintainers, bevy just has its own timelines and needs to take care of) it was decided to fork that repository. The following steps were taken:
  - mikktspace was forked to https://github.com/bevyengine/mikktspace in order to preserve the repository's history in case the original is ever taken down
  - The README in that repo was edited to add a note stating from where the repository was forked and explaining why
  - The repo was locked for changes as its only purpose is historical
  - The repo was integrated into the bevy repo using `git subtree add --prefix crates/bevy_mikktspace git@github.com:bevyengine/mikktspace.git master`
  - In `bevy_mikktspace`:
    - The travis configuration was removed
    - `cargo fmt` was run
    - The `Cargo.toml` was conformed to bevy's (just adding bevy to the keywords, changing the homepage and repository, changing the version to 0.7.0-dev - importantly the license is exactly the same)
    - Remove the features, remove `nalgebra` entirely, only use `glam`, suppress clippy.
      - This was necessary because our CI runs clippy with `--all-features` and the `nalgebra` and `glam` features are mutually exclusive, plus I don't want to modify this highly numerically-sensitive code just to appease clippy and diverge even more from upstream.
- Rebase #1795
  - @jakobhellermann said it was fine to copy and paste but it ended up being almost exactly the same with just a couple of adjustments when validating correctness so I decided to actually rebase it and then build on top of it.
- Use the exact same fragment shader code to ensure correct normal mapping.
- Tested with both https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentMirrorTest which has vertex tangents and https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentTest which requires vertex tangent generation

Co-authored-by: alteous <alteous@outlook.com>
@bors bors bot changed the title Generate vertex tangents using mikktspace [Merged by Bors] - Generate vertex tangents using mikktspace May 31, 2022
@bors bors bot closed this May 31, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective

Models can be produced that do not have vertex tangents but do have normal map textures. The tangents can be generated. There is a way that the vertex tangents can be generated to be exactly invertible to avoid introducing error when recreating the normals in the fragment shader.

## Solution

- After attempts to get https://github.com/gltf-rs/mikktspace to integrate simple glam changes and version bumps, and releases of that crate taking weeks / not being made (no offense intended to the authors/maintainers, bevy just has its own timelines and needs to take care of) it was decided to fork that repository. The following steps were taken:
  - mikktspace was forked to https://github.com/bevyengine/mikktspace in order to preserve the repository's history in case the original is ever taken down
  - The README in that repo was edited to add a note stating from where the repository was forked and explaining why
  - The repo was locked for changes as its only purpose is historical
  - The repo was integrated into the bevy repo using `git subtree add --prefix crates/bevy_mikktspace git@github.com:bevyengine/mikktspace.git master`
  - In `bevy_mikktspace`:
    - The travis configuration was removed
    - `cargo fmt` was run
    - The `Cargo.toml` was conformed to bevy's (just adding bevy to the keywords, changing the homepage and repository, changing the version to 0.7.0-dev - importantly the license is exactly the same)
    - Remove the features, remove `nalgebra` entirely, only use `glam`, suppress clippy.
      - This was necessary because our CI runs clippy with `--all-features` and the `nalgebra` and `glam` features are mutually exclusive, plus I don't want to modify this highly numerically-sensitive code just to appease clippy and diverge even more from upstream.
- Rebase bevyengine#1795
  - @jakobhellermann said it was fine to copy and paste but it ended up being almost exactly the same with just a couple of adjustments when validating correctness so I decided to actually rebase it and then build on top of it.
- Use the exact same fragment shader code to ensure correct normal mapping.
- Tested with both https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentMirrorTest which has vertex tangents and https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentTest which requires vertex tangent generation

Co-authored-by: alteous <alteous@outlook.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Models can be produced that do not have vertex tangents but do have normal map textures. The tangents can be generated. There is a way that the vertex tangents can be generated to be exactly invertible to avoid introducing error when recreating the normals in the fragment shader.

## Solution

- After attempts to get https://github.com/gltf-rs/mikktspace to integrate simple glam changes and version bumps, and releases of that crate taking weeks / not being made (no offense intended to the authors/maintainers, bevy just has its own timelines and needs to take care of) it was decided to fork that repository. The following steps were taken:
  - mikktspace was forked to https://github.com/bevyengine/mikktspace in order to preserve the repository's history in case the original is ever taken down
  - The README in that repo was edited to add a note stating from where the repository was forked and explaining why
  - The repo was locked for changes as its only purpose is historical
  - The repo was integrated into the bevy repo using `git subtree add --prefix crates/bevy_mikktspace git@github.com:bevyengine/mikktspace.git master`
  - In `bevy_mikktspace`:
    - The travis configuration was removed
    - `cargo fmt` was run
    - The `Cargo.toml` was conformed to bevy's (just adding bevy to the keywords, changing the homepage and repository, changing the version to 0.7.0-dev - importantly the license is exactly the same)
    - Remove the features, remove `nalgebra` entirely, only use `glam`, suppress clippy.
      - This was necessary because our CI runs clippy with `--all-features` and the `nalgebra` and `glam` features are mutually exclusive, plus I don't want to modify this highly numerically-sensitive code just to appease clippy and diverge even more from upstream.
- Rebase bevyengine#1795
  - @jakobhellermann said it was fine to copy and paste but it ended up being almost exactly the same with just a couple of adjustments when validating correctness so I decided to actually rebase it and then build on top of it.
- Use the exact same fragment shader code to ensure correct normal mapping.
- Tested with both https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentMirrorTest which has vertex tangents and https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/NormalTangentTest which requires vertex tangent generation

Co-authored-by: alteous <alteous@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds A-Rendering Drawing game state to the screen C-Enhancement A new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet