Naming scheme change for meshes imported from .glb breaking scenes

lostminds commented 1 year ago

Godot version

v4.2.beta1.official [b1371806a]

System information

Godot v4.2.beta1 - macOS 13.6.0 - Vulkan (Mobile) - integrated Apple M2 Pro - Apple M2 Pro (10 Threads)

Issue description

It appears 4.2.beta1 has introduced a new internal mesh format, and when you first open a project from a previous version you get a warning that all the meshes have been re-imported, and if you save them like this they'll be incompatible with previous versions. servers/rendering_server.cpp:2035 - Upgrading mesh from older surface format. Once saved again (or re-imported), this mesh will be incompatible with earlier versions of Godot. (User)

However, it seems that this new mesh import has also broken all my meshes due to a naming issue: This might have been a bug in the previous importer, but for some reason in the imported mesh scenes of my .glb (and hence in the scenes instancing these) the meshes often have "2" after their name. So, for example the Block.glb file import I've supplied as an example resulted (pre 4.2beta1) in an imported scene with a MeshInstance3D name called "Block2". But the new format importer instead results in a MeshInstance3D called simply "Block". This new name in turn means all references to the mesh in the scene (as far as I can tell in this project, all references to all meshes in all scenes) break, since they are overrides, node_paths etc for "Block2" and this no longer exists in the node hierarchy.

scene/resources/packed_scene.cpp:231 - Node 'Model/Block/Block2' was modified from inside an instance, but it has vanished

Steps to reproduce

Import a glb file in a pre 4.2.beta1 (or maybe earlier?) godot project (applied example file below).
Instance this in a scene. Notice that the MeshInstance3D in the imported scene is called "Block2"
Make some reference to this in the scene, for example set an override material or make a script referencing the mesh as an export var.
Open the project in 4.2beta1
Observer that the new re-imported MeshInstance3D is now called just "Block" instead of "Block2", and that the references/overrides in your scene are broken.

Minimal reproduction project

Test model file: Block.glb.zip

AThousandShips commented 1 year ago

This is expected I think, this was a bug that got fixed in:

80270

lostminds commented 1 year ago

Ah, yes, I suspected as much since the old "2"-named looked like a bug. It's basically broken all 3d elements and material assignments in this project though, and I don't think there's an easy way to fix this in Godot? Also, https://github.com/godotengine/godot/pull/80270#issuecomment-1703221933 is not correct it seems. This file was made and exported from Blender, so Blender-generated GLTF-files are not safe from this.

AThousandShips commented 1 year ago

The Blender aspect of that is a good point, worth investigating how to handle that

akien-mga commented 1 year ago

I can confirm the issue on https://github.com/gdquest-demos/godot-4-3d-third-person-controller

One of its glb's triggers this error:

WARNING: Node 'bee_bot/Armature/Skeleton3D/bee_bot2' was modified from inside an instance, but it has vanished.
     at: instantiate (scene/resources/packed_scene.cpp:231)

For that project specifically it's relatively easy to workaround with:

sed -i $(rg bee_bot2 -l) -e "s/bee_bot2/bee_bot/g"

But not all projects use only text-based scenes that can be grepped like this, and it will break compatibility with 4.1.x.

aaronfranke commented 1 year ago

This is happening because the file name and mesh node name match each other exactly. In Godot 4.1.x, if you rename this file to "MyBlock.glb", then it will generate a mesh named "Block".

To be clear, the old behavior is plain wrong. Renaming a file should not change how it's imported.

As for working around this, I'm not sure what's best.

lostminds commented 1 year ago

I agree that the old naming was very wrong, so we shouldn't go back to that even if it would be convenient. Looking at the .import files they reference what I assume is the old imported scene for the glb since it has been imported before.

path="res://.godot/imported/Block.glb-669b8c1eaacc54efac76a0583f41ed92.scn"
dest_files=["res://.godot/imported/Block.glb-669b8c1eaacc54efac76a0583f41ed92.scn"]

These aren't text scene files, so I can't read them myself, but if they contain the previous loaded scene result it should be possible to at least see what the resulting mesh nodes were named previously? If this is so it could at least be a starting point to detect if a re-import (any re-import, not just this case) has resulted in nodes being renamed. Perhaps it would be a good idea in all such cases to then prompt the user and offer to change any references to these meshes accordingly in any scenes instantiating the imported mesh.

I'm guessing this would be a good thing if you intentionally/unintentionally changed the name of a mesh in Blender and then re-imported the file getting the same issue with the mesh being renamed and scene references broken, so it wouldn't just be to fix this rename case.

dc1248 commented 1 year ago

This was mentioned in the release notes https://godotengine.org/article/dev-snapshot-godot-4-2-beta-1/#breaking-changes

"We are looking for a way to preserve compatibility for existing assets and should introduce it in later beta releases."

I guess when reimporting, the importer could try to match up orphaned nodes with similar names and the same type. There could be an import option to preserve the old names. This should be possible even if you delete your .godot cache folder - you still have your imported .tscn files and .glb.import files.

lostminds commented 1 year ago

From https://github.com/godotengine/godot/pull/83613

For the linked issue we want to introduce a way to version importer behavior, so you can keep the old logic for existing scenes.

Perhaps an easier way to do this could be to add override-properties to the .import file format for resources. When the importer detects a change in properties that could break references (for example mesh node name) the user can be asked if they want to accept this change or retain the old value. If they chose to retain the value an override property is added to the .import file that says to change for example mesh name back to "Block2" again, regardless of what the file import results in. Ensuring the import result is the same as it was last import. This would mean that we don't need to keep track of how different versions imported assets or what version imported this. But we can still retain compatibility when changes happen. The same system could also catch unintentional mesh renames in files imported in the same version. And as these override names/properties could be visible in the import panel interface it can help the user understand how the system works and change/remove these overrides if they know what they're doing.

rcorre commented 1 year ago

On main, the root nodes of files imported using the blender importer have a hash after the name, e.g. Character-e1c248ae26fc8808362fd6c8fbab07b3. I don't have the same issue with a .glb import of the same file.

Is that related, or should I file a separate issue? example.zip

dc1248 commented 1 year ago

@rcorre That sounds like a separate/additional issue. The blender importer works by running blender in batch mode to do a gltf export, then godot uses its gltf importer, but it may do some post-processing.

Personally I just use gltf/glb to keep things simple, as recommened by the godot docs.

godotengine / godot