-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Alternative glTF coordinate conversion #20394
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
cart
merged 40 commits into
bevyengine:main
from
greeble-dev:gltf-coordinate-conversion
Dec 16, 2025
+244
−153
Merged
Changes from all commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
04ec20a
Removed `gltf_convert_coordinates_default` feature and Bevy 0.18 warn…
greeble-dev a67c1e9
Changed `convert_coordinates` from a bool to a `GltfConvertCoordinate…
greeble-dev 391741f
Removed node conversion.
greeble-dev 1cf0d3f
Removed camera and rotation conversions that were only used by node c…
greeble-dev 3d2bf55
Added scene conversion.
greeble-dev 6625359
Fixed skinned mesh inverse bindposes. The Mat4 conversion assumed tha…
greeble-dev 13d13b5
Mesh conversion now rotates the mesh primitive entities. This negates…
greeble-dev 080c3bc
Added TODOs.
greeble-dev 25d8992
Added utility functions for conversion transforms.
greeble-dev 592e076
More TODOs.
greeble-dev 20b3c3a
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 0042c67
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 63b16ac
Updated documentation. Fixed `coordinate_conversion` not being a publ…
greeble-dev f24c988
Renamed `GltfConvertCoordinates` variable from `scene` to `scenes`. T…
greeble-dev 7984aa6
Removed the awkwardly special case `ConvertInverseCoordinates` in fav…
greeble-dev 5cde975
Updated docs. Clarified that the mesh entity actually gets the invers…
greeble-dev 0b4235e
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 9248860
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 415b989
Fixed cases that were still using `use_model_forward_direction`.
greeble-dev e0e777f
Updated release notes.
greeble-dev b24b258
Fixed typo
greeble-dev fb77cfb
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 62f4018
Updated morph and bounds conversions.
greeble-dev b2567a5
Updated examples.
greeble-dev 3369ef0
Reverted all changes to the 0.17 release note.
greeble-dev 292dad0
Added new migration guide for 0.18.
greeble-dev f02a9e2
Fixed markdown lint.
greeble-dev b82be31
Improved comment.
greeble-dev 9e9dea5
Fixed markdown lint for real this time... maybe?
greeble-dev 252f1cc
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 9eed9ce
Improved migration guide.
greeble-dev 988746f
Fixed markdown lint.
greeble-dev 9b74297
Tweaked migration guide to be more explicit about which options the u…
greeble-dev acd10e8
`rotate_scene_entity` and `rotate_meshes`
cart 05fadb7
Changed variable names in migration guide.
greeble-dev 1123386
Reverted accidental commit.
greeble-dev f545dab
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev f83a7ae
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev 4b565e2
Merge branch 'main' into gltf-coordinate-conversion
greeble-dev f9552c1
Delete stray temp.txt
alice-i-cecile File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like the big downside of this new approach is that local transforms within a scene (that correspond to nodes) will now no longer have "correct" forward directions. This relies on nesting the scene underneath a top level entity that is "uncorrected", which we use to call "forward" on. Trying to do that within the scene will not produce the expected "forward" behavior.
This is likely to be problematic in the world of BSN "scene inheritance", where rather than spawning the gltf scene root underneath a higher level "bevy scene root", we would instead treat the gltf scene root as the root, and write our changes directly on top. I believe this is the better model when compared to the current "add another root on top" approach, as it allows developers to think about and interact with GLTF scenes exactly as if they were bevy scenes (ex: if the root node has a property, then when we inherit from it, we can directly read/write that property on the root).
I believe this will also come into play if developers try to animate the root transform of the scene, which will snap our corrections back to whatever the animation thinks is correct.
The benefit of course is that the scene is "locally consistent". But within that space, everything is still "uncorrected". If a "player" node exists inside a larger GLTF "world" scene, that player scene's forward will be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got one thing to check before I reply in full - does BSN require a single scene root entity or does it support multiple roots? Asking because glTF supports multiple roots, so if BSN doesn't then that could spoil things.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, gonna reply now so we can keep moving forward - I'll assume that BSN does support multiple roots.
There's something I should have clarified in the PR: dropping support for node conversion in the short-term doesn't stop us adding it back in later. There are some awkward questions around cameras (which the 0.17 conversion has to address too), but if they're solved then it's feasible to support "scene conversion" and "node conversion" as mutually exclusive options. I've edited the PR to add more detail on this - see the "Future" section at the bottom.
Having two options might seem more complex than necessary. But I think it reflects that different users will have different ideas of what the correct semantics are, and which parts of the scene they prefer to modify.
So right now the 0.17 conversion could be framed as "support one use case, but with bugs". While this PR is "support a different use case, without bugs". Both choices have paths to supporting the other's use case. But I feel this PR is the least worst choice in the short-term, and solves the more common use case of current users.
Another path forward is to try solving the camera problems first, then come back to scene conversion versus node conversion later. I can expand on this solution if desired.
That behaviour can be supported by this PR if the correction is baked into the glTF root nodes instead of the current scene root.
That issue can be solved by this PR if the animation of root nodes is corrected. Note that 0.17 and node conversion in general already requires correcting animations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and no. BSN currently has two forms:
bsn!andbsn_list!. These are separate things becausebsn!can then be a single root / directly inheritable / directly writeable on top andbsn_list!can be spawned flat as many roots or as a relationship. We're also discussing "BSN sets", which would be collections of multiple named BSN entries.I think we'll want to latch on to one of the two models for glTF assets (or perhaps load it as a BSN set and force developers to select individual scenes from it).
I don't object to supporting both paths, provided they are clearly labeled. I think node conversion is likely the "endgame" and I think I actually prefer leaving the current node conversion code in, but putting it under an experimental flag with docs explaining the situation and limitations. No need to throw it out entirely / start from scratch. It seems like the limitations are a result of not doing some things, rather than doing things incorrectly.
I don't think thats true in all cases. The current corrective behavior only works because we have a root node that is "uncorrected / identity" that we call forward on. Doing the correction at the root removes that, so forward would still be mismatched.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I disagree, but I might be just misunderstanding how BSN works. The "scene conversion" option in this PR is basically "do whatever is necessary to make it visually correct but without changing node semantics". I think that can work however BSN organizes the hierarchy, even if some cases are more complex (like needing animation fixup). But maybe this discussion should wait until BSN lands and I can understand it better.
On node conversion being the endgame, I don't agree. It's a useful option, but I think some users will also want the option of whole scene conversion that doesn't change node semantics. Maybe that's how their assets ended up, or maybe they prefer those semantics, or maybe they feel more comfortable with a more minimal option.
Moving forward, I've sketched out a PR that might more agreeable: greeble-dev#1. This keeps the current node conversion and adds a mutually exclusive scene conversion option. I don't like this, because I think it's risky to continue shipping a quirky node conversion when users might come to depend on the quirks, and the path to reliable node conversion remains unclear. But I can follow it through to a real PR if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that "do whatever" currently relies on adding an additional layer of hierarchy when spawning in Bevy to provide the "correct" forward direction. If we remove that, then we can't "do whatever" to fix it without changing the node semantics. This isn't a problem currently, but it will surface the moment we remove that added layer of hierarchy (and I strongly think that we should).
I'm cool with "no node conversion, reliable scene conversion" path for 0.18. I just expect us to very quickly feel the need to reopen this conversation (and reintroduce essentially all of the existing node conversion code) when BSN lands and we drop the extra hierarchy layer.
I don't love this framing for the current implementation. I think a more functional description like
rotate_scene_rootwould help people conceptualize this better than something like "scene conversion" which could be interpreted to be a more holistic conversion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm still confused about the hierarchy issues, but I'll assume we don't have to resolve that now.
I'm happy to develop greeble-dev#1 into an alternative PR if a maintainer thinks that might help move things forward or make decisions clearer.
I'm reluctant to use the word "root" as (pre-BSN at least) that could be misinterpreted as the glTF root node(s) or the entity with the
SceneRootcomponent.I could call it
rotate_scenes. But the catch is that the separate mesh and (future) node conversions are more complex, and arguably a functional description won't fit in their names - they've got more going on than a single rotation. So we'd probably end up with an awkwardstruct GltfCoordinateConversion { rotate_scenes, meshes, nodes }.Or we add
rotateto all of them, even if it's a bit approximate. So I favor the current state, but I'm fine to change it torotate_scenes, rotate_meshesif you prefer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a proposed
rotate_scene_entity(this matches the terminology in the docs and solves the "root ambiguity" issue) androtate_meshesrename. I really don't think high level / general purpose names are appropriate here. Open to alternatives though!I also added some additional clarity to the
rotate_scene_entitydocs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down to merge this for 0.18 if y'all are.