Clarify inconsistent GltfConvertCoordinates 0.18 behavior

[ChristopherBiscardi]

Objective

When using GltfConvertCoordinates as of #20394 , which is 0.18's version, using the flags results in the forward vector moving in a different direction for various entities in the hierarchy.

This is confusing, and (in my opinion) a broken behavior since it relies on users only ever moving the scene root or mesh entities forward. It is very common that a player's object is not one mesh. In fact this is the default case of the default Blender scene. The default cube turns into

• Scene
  • Parent Entity
    • Mesh3d/Material Entity

When using these flags the "Parent Entity"'s forward vector is the one that would be targeted to move the cube, since that is the root of the object. This Entity's forward direction would be in the opposite direction of the forward direction for the mesh, resulting in different entities in the hierarchy moving in different directions with no way to identify which will behave in which way.

Solution

No fix. Just Documentation. The flag is experimental afaik and changes fall under being experimental.

[janhohenheim]

Suggested change

	///
	/// Setting any of these values to true will result in the relevant entities
	/// moving in opposite directions when using the forward direction.
	/// This means that within a scene, different entities in a hierarchy will
	/// move in different global directions when using the forward direction
	/// for that `Entity`.
	///
	/// For example, a parent `Entity` A, with a child `Entity` B that has a `Mesh3d`
	/// component:
	///
	/// - Entity A with a `Transform`
	/// - Entity B with a `Transform`, `Mesh3d`, `MeshMaterial3d`
	///
	/// ## Disclaimer
	///
	/// Setting any of these values to true will result in the relevant entities
	/// moving in opposite directions when using the forward direction.
	/// This means that within a scene, different entities in a hierarchy will
	/// move in different global directions when using the forward direction
	/// for that `Entity`.
	///
	/// For example, a parent `Entity` A, with a child `Entity` B that has a `Mesh3d`
	/// component:
	///
	/// - `SceneRoot`
	/// - Entity A with a `Transform`
	/// - Entity B with a `Transform`, `Mesh3d`, `MeshMaterial3d`

[janhohenheim]

Suggested change

	///
	/// Setting any of these values to true will result in the relevant entities
	/// moving in opposite directions when using the forward direction.
	/// This means that within a scene, different entities in a hierarchy will
	/// move in different global directions when using the forward direction
	/// for that `Entity`.
	///
	/// For example, a parent `Entity` A, with a child `Entity` B that has a `Mesh3d`
	/// component:
	///
	/// - Entity A with a `Transform`
	/// - Entity B with a `Transform`, `Mesh3d`, `MeshMaterial3d`
	///
	/// If A's forward moves "North", B's forward moves "South"
	///
	/// ## Disclaimer
	///
	/// Setting any of these values to true will result in the relevant entities
	/// moving in opposite directions when using the forward direction.
	/// This means that within a scene, different entities in a hierarchy will
	/// move in different global directions when using the forward direction
	/// for that `Entity`.
	///
	/// For example, a parent `Entity` A, with a child `Entity` B that has a `Mesh3d`
	/// component:
	///
	/// - `SceneRoot`
	/// - Entity A with a `Transform`
	/// - Entity B with a `Transform`, `Mesh3d`, `MeshMaterial3d`
	///
	/// If A's forward moves "North", B's forward moves "South"

[ChristopherBiscardi]

SceneRoot's placement here is incorrect. The tree doesn't even have to include it if spawned from a scene_spawner, and writing it like this makes it confusing as to whether it's referring to the SceneRoot or the scene's root, which are different entities.

[greeble-dev]

I'd say this behaviour is already documented on the options:

pub struct GltfConvertCoordinates {
    /// If true, convert scenes by rotating the top-level transform of the scene entity.
    ///
    /// This will ensure that [`Transform::forward`] of the "root" entity (the one with [`SceneInstance`](bevy_scene::SceneInstance))
    /// aligns with the "forward" of the glTF scene.
    ///
    /// The glTF loader creates an entity for each glTF scene. Entities are
    /// then created for each node within the glTF scene. Nodes without a
    /// parent in the glTF scene become children of the scene entity.
    ///
    /// This option only changes the transform of the scene entity. It does not
    /// directly change the transforms of node entities - it only changes them
    /// indirectly through transform inheritance.
    pub rotate_scene_entity: bool,

    /// If true, convert mesh assets. This includes skinned mesh bind poses.
    ///
    /// This option only changes mesh assets and the transforms of entities that
    /// instance meshes. It does not change the transforms of entities that
    /// correspond to glTF nodes.
    pub rotate_meshes: bool,
}

So they both state which entities they convert and which are left alone.

Maybe that's not sufficient, and they should go into more detail on what situations the behavior can create.

[ChristopherBiscardi]

I feel it is important to indicate that the current flag's behavior results in forward being inconsistent within a scene.

[greeble-dev]

I'm going to think this through, and probably draft an alternative. Might not be until early January though.

I don't think this PR should land as is, since it makes assumptions about how users have set up their glTFs. Also any clarification should be on the GltfConvertCoordinates struct, not on GltfLoader and GltfLoaderSettings .

[greeble-dev]

I've drafted an alternative here: https://github.com/greeble-dev/bevy/pull/2/changes

It doesn't go into much detail. I find it hard to explain this stuff without doing a full breakdown of how a glTF is turned into entities. Even just the "SceneRoot entity -> scene entity -> gltf root node entity" setup is confusing. So it doesn't do much more than say "there might be inconsistencies".

I did consider adding a section that says "if in doubt, just enable rotate_scene_entity". Since for users who only care about the scene as a whole, that will do the right thing for most glTFs.

[ChristopherBiscardi]

since it makes assumptions about how users have set up their glTFs

Its not clear which assumptions are being considered invalid here. The nodes are not converted and the meshes/scenes are, which results in some entities, which all had default Transforms prior to conversion, having forward behavior that is different than others, especially given a default glTF asset. To not have this happen would, as far as I can tell, require a user to have rotated every node in their project but not rotated the meshes.

That is to say, we don't currently have an example of when using these flags would result in "correct" node forward movement, so catering docs to a use case that we don't have an example of, and doesn't get produced by common DCC software like Blender, seems strange.

I also think that its pretty safe to say "a gltf exported from blender" represents a reasonable default to center documentation around, and this "reversed" behavior is the behavior that occurs with the default scene.

I find it hard to explain this stuff without doing a full breakdown of how a glTF is turned into entities

It feels like this feature should have merged with an explanation of this in one of the PRs tbh, especially if that is the case. There have been multiple cases during development of one type being converted and another not being converted that should've been evaluated before shipping this feature.

Right now this is an experimental feature that had breaking, non-migratable changes between 0.17 and 0.18, and doesn't seem to have the aforementioned documentation. Which is mostly an issue because the feature isn't labelled as experimental and people have started using it in their projects.

This feature also doesn't seem to account for use cases where an object might get "picked up" and taken out of the rotated scene root.

"SceneRoot entity -> scene entity -> gltf root node entity"

I'm not sure what you mean by "gltf root node entity". The SceneRoot is an optional parent container for the scene instance (If you used SceneRoot instead of scene_spawner), the scene entity is a child of that (if spawned with SceneRoot). So I'm not sure what you mean by "gltf root node entity" since the scene entity is already the root.

---

I was originally going to file an issue instead of a docs change to have something to link to when people ran into this issue (it was suggested that I make a docs PR instead), so I'm not super tied to this particular PR. If docs change here isn't something that is going to get content feedback for merge and you'd rather move forward with alternatives then I'm fine with closing it and using this PR as that link.

The other PR should actually get put up against the bevy repo for merge though so it gets included in 0.18. I do feel like it hides the fact that this forward-reversal issue occurs with a bog-standard gltf export from blender behind "some gltf files might not follow the standard" which afaik doesn't actually apply to that case.

[greeble-dev]

we don't currently have an example of when using these flags would result in "correct" node forward movement

There is one case where rotate_scene_entity will give the right result - if the user had their scene +Z forward but their nodes -Z forward. Maybe that's a bit nitpicky, and the number of users that need it might well be zero, but it's possible to create that situation in Blender.

You're right that the rotate_meshes option cannot result in correct node semantics.

"SceneRoot entity -> scene entity -> gltf root node entity"

I'm not sure what you mean by "gltf root node entity".

The entity that corresponds to the root node (or nodes) of the gltf scene. So if I have a blender scene with a single cube object, then the entities will be "SceneRoot/SceneInstance entity -> scene entity -> cube node entity".

The current docs do kinda go over this but not in much detail. And they avoid using the word "root" because of the ambiguity.

/// The glTF loader creates an entity for each glTF scene. Entities are
/// then created for each node within the glTF scene. Nodes without a
/// parent in the glTF scene become children of the scene entity.
///
/// This option only changes the transform of the scene entity. It does not
/// directly change the transforms of node entities - it only changes them
/// indirectly through transform inheritance.

Right now this is an experimental feature that had breaking, non-migratable changes between 0.17 and 0.18, and doesn't seem to have the aforementioned documentation. Which is mostly an issue because the feature isn't labelled as experimental and people have started using it in their projects.

The 0.17 conversion was documented as experimental, since it was known to have bugs. The 0.18 conversion is not documented as experimental - it only satisfies some use cases, and the behavior might be unintuitive, but it doesn't have (known) bugs. It's also designed to minimise the risk of future breaking changes. That said, I'm open to documenting the 0.18 conversion as experimental out of an abundance of caution.

The other PR should actually get put up against the bevy repo for merge though so it gets included in 0.18.

I'm afraid I've run out of time to make a proper PR. I'm assuming there'll be some time after the holidays.

[ChristopherBiscardi]

ok so to summarize:

• If the user has a glTF which rotated all of their scene nodes, but not the scene itself, then rotate_scene_entity could result in the forward direction matching between the scene and the nodes.
  • Additionally, the "corrected rotation" relies on being a child of the rotated scene. If an entity is removed or inserted into the scene, such as picking up a box, that is not handled.
• The rotate_meshes option cannot result in correct node semantics.
• In the default case, using the rotate_meshes and rotate_scene_entity flags results in nodes moving in the opposite direction of scenes and meshes when using Transform::forward

---

For the future PR, in the current docs, I'm pretty sure this line is wrong. All nodes in a scene are already part of the scene. There are no nodes in a scene without a parent, since the glTF scene structure specifies the top level nodes and they specify their children. The loader doesn't have any logic for "parentless nodes" as far as I can see.

Nodes without a parent in the glTF scene become children of the scene entity.

I'll close this, since it doesn't seem on track to be merged, and hope that an alternative is put forth.

[greeble-dev]

For the future PR, in the current docs, I'm pretty sure this line is wrong. All nodes in a scene are already part of the scene. There are no nodes in a scene without a parent, since the glTF scene structure specifies the top level nodes and they specify their children. The loader doesn't have any logic for "parentless nodes" as far as I can see.

Nodes without a parent in the glTF scene become children of the scene entity.

This is me trying to avoid the ambiguity over the word "root", and hoping the user understands that "glTF scene nodes without a parent" probably means "the root they see in their DCC". But I don't want to say "DCC root" because there might be DCCs or glTF exporters that add a dummy root for scaling reasons. It's a mess.

I think the end goal should be to write guides for each DCC, so we can say "this is exactly what happens if you're using Blender and you should probably choose options X and Y". But I'm not sure doc comments are the right place for that - should be in a more cohesive wiki/tutorial like Godot.

[ChristopherBiscardi]

I feel like we're trying to cater to edge cases that might not exist at the cost of documenting regular cases here. We don't need to explain DCC exports because this is a glTF conversion feature. A scene in glTF is the root and there are no unparented nodes in a scene, so the statement that "Nodes without a parent in the glTF scene become children of the scene entity." doesn't make sense in the context of a glTF file.