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

Fix doubly-reserved unique names in GLTF scene name assignment #80270

Merged

Conversation

aaronfranke
Copy link
Member

This PR has 2 related but technically independent bugfixes. I could separate them but I figured I'd just make one PR. These fixes very slightly change behavior, so do not cherry-pick this to 4.1.x, but it's good for 4.2.

  1. Fix double reserving of node names for nodes without a name in _assign_node_names. Test file:
{
	"asset": { "version": "2.0" },
	"nodes": [{}],
	"scene": 0,
	"scenes": [{ "nodes": [0] }]
}

Behavior in master:
Screenshot 2023-08-04 at 12 32 00 PM

Behavior in this PR:
Screenshot 2023-08-04 at 12 26 57 PM

  1. Fix scene names in _parse_scenes reserving as priority over node names and breaking node names. Test file:
{
	"asset": { "version": "2.0" },
	"nodes": [{ "name": "EmptyScene" }],
	"scene": 0,
	"scenes": [{ "nodes": [0], "name": "EmptyScene" }]
}

Behavior in master:
Screenshot 2023-08-04 at 12 32 58 PM

Behavior in this PR:
Screenshot 2023-08-04 at 12 16 29 PM

Note: In the future I do want the scene name to be used as the root node name in some situations. However, in the current master this does not happen, and in the future when we do want this to happen, the current part of the code reserving the name is the wrong place to do it. We want to do it later on so that the actual nodes in the GLTF file have their names take priority, and the names of the nodes Godot generates are secondary.

@fire
Copy link
Member

fire commented Aug 4, 2023

Is there a reason for the name changes? Like Camera3D

@aaronfranke

This comment was marked as resolved.

@fire
Copy link
Member

fire commented Aug 7, 2023

I don't have a strong opinion on the names, but it is a backwards compatibility change I think.

It's not common to animate cameras or lights, but it's possible.

@aaronfranke

This comment was marked as resolved.

@aaronfranke

This comment was marked as resolved.

@fire
Copy link
Member

fire commented Aug 20, 2023

I think the other changes are good, but we're discussing how to arrange the name change.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

Summary

  • Name change from s to scene_dict (no operation / code cleaness)
  • Remove _gen_unique_name on the gltf node name

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

		if (s.has("name") && !String(s["name"]).is_empty() && !((String)s["name"]).begins_with("Scene")) {
			p_state->scene_name = _gen_unique_name(p_state, s["name"]);
		} else {
			p_state->scene_name = _gen_unique_name(p_state, p_state->filename);
		}

This part has the _gen_unique_name removed as it was a bug.

There is a rename from s to scene_dict and caching the gltf_node_name.

@akien-mga
Copy link
Member

What's the impact on compatibility for scenes imported in 4.1, and with changes to the nodes which would get a rename?

I already had to deal with such a compat breakage between 4.0 and 4.1 in gdquest-demos/godot-4-3d-third-person-controller#30, and that wasn't pretty. We should do our best to avoid breaking projects as far as possible.

@aaronfranke
Copy link
Member Author

aaronfranke commented Sep 1, 2023

@akien-mga I actually tested on that exact project last week. Every node in every GLB file in that project is named, therefore there are no auto-generated names, therefore this PR does not affect that project at all.

I think most people use GLB files exported from Blender, and Blender requires all nodes to have unique names, so this PR should not affect any GLB file exported from Blender.

@akien-mga akien-mga merged commit 7f3dbe8 into godotengine:master Sep 2, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants