Skip to content

Enable glTF coordinate conversion for our examples #20099

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

Open
wants to merge 52 commits into
base: main
Choose a base branch
from

Conversation

janhohenheim
Copy link
Member

@janhohenheim janhohenheim commented Jul 12, 2025

Objective

Solution

  • Let's enable the new coordinate system conversion as part of [dev-dependencies] and port all of our examples!
  • The course of action was selected on a case by case basis
    • Sometimes, the environment map used for decorative lighting or the background would be flipped. That doesn't matter at all for the example, so I allowed the slight visual change.
    • Sometimes, nothing needed to be changed. A model would be flipped, but the example looked just as good and conveyed the same information, so no need to touch it.
    • Sometimes, a model was clearly meant to face the camera. For nearly all cases, I rotated the model to face the camera, as keeping the camera positioned the "usual way" seems like the most idiomatic choice
      • For mixed_lighting, that would intrude on other code that assumed that the Transform of the mesh was equal to its GlobalTransform, so I rotated the camera instead
      • For the scene viewer, it seemed more logical to import the scenes as-is and rotate the camera instead to face them
      • reflection_probes required a bit of an ugly fix, but this one is undergoing a major change in its cubemaps anyways in an upcoming PR, so no need to touch it here. See Environment Map Filtering GPU pipeline #19076. That PR will make it nicer again :)
  • I also fixed an issue where glTF lights were rotated the wrong way that I discovered while porting
    • I verified that this light handling is correct with a dedicated Blender test scene not included in this PR
  • Notable corollary: meshlets are now more different from glTF assets, as they use Bevy's coordinate system, not glTF's
    • Jasmine says this is the correct behavior

Testing

  • I went through all 84 instances of the regex \.gl(tf|b) in the examples directory and compared them all with their output without the new feature

@janhohenheim janhohenheim added this to the 0.17 milestone Jul 12, 2025
@janhohenheim janhohenheim added C-Examples An addition or correction to our examples A-glTF Related to the glTF 3D scene/model format D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 12, 2025
@janhohenheim janhohenheim changed the title Port gltf examples Enable glTF coordinate conversion for our examples Jul 12, 2025
@janhohenheim janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 13, 2025
@janhohenheim janhohenheim marked this pull request as ready for review July 13, 2025 01:20
@janhohenheim janhohenheim requested a review from mate-h July 13, 2025 01:31
@janhohenheim janhohenheim added the X-Uncontroversial This work is generally agreed upon label Jul 13, 2025
@janhohenheim
Copy link
Member Author

Marking as uncontroversial because

  • This migration path is the one that Cart favored
  • The controversial part was already merged (i.e. showing the migration warning)
  • We obviously don't want to have examples with deprecated code in them

Copy link
Contributor

@mate-h mate-h left a comment

Choose a reason for hiding this comment

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

The whole diff looks good to me, I have no nitpicks about it. I tried converting some assets to use this new coordinate system but determined that would take too much effort to be in scope of this PR. Thus, we will do it incrementally on an example-by-example basis instead.
I spot checked a couple of examples (specifically where baked lighting direction matters), maybe we can check pixel eagle and see what remaining examples pixel output have changed.

@andriyDev andriyDev added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 13, 2025
@janhohenheim janhohenheim removed the request for review from alice-i-cecile July 13, 2025 02:46
@janhohenheim janhohenheim added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 13, 2025
@janhohenheim
Copy link
Member Author

janhohenheim commented Jul 13, 2025

@janhohenheim
Copy link
Member Author

janhohenheim commented Jul 13, 2025

Reopening since the community vibe seems to be that these changes are still the best way forward, but we should document it better: #20121
Still leaving this as uncontroversial because the controversial bit was #19816, not this followup

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-glTF Related to the glTF 3D scene/model format C-Examples An addition or correction to our examples D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants