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

Use the Blender file name instead of the generated GLTF file name #84678

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Nov 9, 2023

Fixes #84025. This was caused by #79774, not #80272. I'll explain what's going on.

In Godot 4.1 and earlier, if a GLTF file had no scene name, the GLTF importer code used the file name as the scene name. This was then overridden by the general scene importer code to be the file name (again). This seemed redundant to me, and I wanted to ensure the GLTF importer has the ability to set the scene name, so any scene names processed by GLTF will be preserved. However, let's look at some scenarios to see what happens:

  • Importing my_scene.glb (assuming no scene name OR "Scene"): The Godot GLTF importer sees no scene name so it sets the scene name to the file name "my_scene". Then the general-purpose importer code overrides this to the file name again "my_scene". Therefore the scene root is named "my_scene".

  • Importing my_scene.blend: Blender exports a GLTF with the scene name set to "Scene" and a temporary file name my_scene-123456etc.glb. The Godot GLTF importer ignores the scene name when it is empty OR set to "Scene", so it sets it to the file name "my_scene-123456etc". Then the general-purpose importer code overrides this to the Blender file name "my_scene". Therefore the scene root is named "my_scene".

In master before this PR, this means that "my_scene" was used in both cases. However #79774 removed the general-purpose importer code overriding the root name, so that importers can set their own root node name. I completely missed that this would change the behavior for Blender file importing.

So, the solution: Update the Blend file import code to set the scene name as the file name before importing the GLTF. The GLTF code will still check for an explicit scene name in the file first, but then if that's not present adjust the GLTF code to prefer any existing scene name if set, instead of using the GLTF file name in that case.

I also checked the FBX importer, and... it's changed in a different way. In Godot 4.1 and earlier it uses "Node3D" as the scene root node name, because the general-purpose importer code overrides this. In master the scene root node name is set to "Root Scene" because that's what the FBX2glTF tool outputs. Both are wrong, we should fall back to the file name just like the Blender and GLTF importers. However in order to fix this I really don't want to add another hack to ignore scene names of "Root Scene", so since we have control over our fork of FBX2glTF I think we should do the proper fix and adjust the FBX2glTF code to allow specifying a scene name.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the quick turnaround!

@akien-mga akien-mga merged commit 9df6491 into godotengine:master Nov 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the blend-name branch November 9, 2023 19:13
@lyuma
Copy link
Contributor

lyuma commented Nov 9, 2023

We should make sure this doesn't affect the naming of extracted meshes in the advanced importer, which currently have the scene name prepended to them.

I believe godot uses the "Root Scene_" prefix for all meshes by default, but I just want to confirm.

@aaronfranke
Copy link
Member Author

aaronfranke commented Nov 9, 2023

@lyuma Yes, this PR does change the behavior when extracting meshes in the advanced import dialog.

Screenshot 2023-11-09 at 2 18 59 PM Screenshot 2023-11-09 at 2 19 11 PM Screenshot 2023-11-09 at 2 19 22 PM

However, the new behavior is superior, and compatibility is not broken, because the paths are stored explicitly:

"meshes": {
"truck_town-8046e9244cc791f60203240e2a4776c7_door_01": {
"generate/lightmap_uv": 0,
"generate/lods": 0,
"generate/shadow_meshes": 0,
"lods/normal_merge_angle": 60.0,
"lods/normal_split_angle": 25.0,
"save_to_file/enabled": true,
"save_to_file/make_streamable": "",
"save_to_file/path": "res://before/truck_town-8046e9244cc791f60203240e2a4776c7_door_01.res"
},

@lyuma
Copy link
Contributor

lyuma commented Nov 9, 2023

The compat breakage would be the key in the meshes array which you can see is still truck_town-8046e9244cc791f60203240e2a4776c7_door_01

This really needs to be addressed in a separate PR though.

The source of this name is actually the "scene"'s name in the glTF document. Unfortunately, Blender and FBX2glTF sometimes put unwanted values there.

In Unidot importer, unfortunately I have to do this:

		var default_scene: Dictionary = json["scenes"][json.get("scene", 0)]
		# Godot prepends the scene name to the name of meshes, for some reason...
		# "Root Scene" is hardcoded in post_import_unity_model.gd so we use it here.
		default_scene["name"] = "Root Scene"

I agree with your assessment within this context, but this would be good to address in 4.3

It might be good to put in the release notes that extracted mesh references may be lost during the engine upgrade, which would increase the size of the embedded meshes in imported scenes unless they are extracted again from the advanced import dialog. does that make sense?

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.

Blender importer adds long suffix to root node name
3 participants