When importing 3d project in 3.4.beta2, materials are not assigned (glTF import regression)

[Arnklit]

Godot version

3.4.beta2

System information

Windows 10, GLES3, GTX 980Ti

Issue description

When importing my cellar project in the beta, some materials are not assigned and reflection probes appear to be wrong.

https://github.com/Arnklit/godot_cellar

Project in 3.3.2.stable

Project in 3.4.beta2

Steps to reproduce

1. Download project https://github.com/Arnklit/godot_cellar
2. Open the project in the beta

Minimal reproduction project

No response

[Calinou]

If you can compile from source, can you try bisecting the regression? This would greatly speed up troubleshooting.

Also, check the Output panel (or, better, the terminal output) for error messages.

[Arnklit]

It does indeed seem to complain quite a lot in the console when I open it.
console_out.txt

I'll see about besecting.

EDIT: I probably won't have time for this until Saturday.

[akien-mga]

I can reproduce the issue in both 3.4 beta 1 and 3.4 beta 2.

I had an old build laying around of a69cc9f (May 20) which imports fine, so the regression is between a69cc9f and beta 1 (207fb16).

There's a lot of errors spammed like this:

ERROR: (Node not found: "../Architecture/Torch8/Torch" (relative to "/root/EditorNode/@@591/@@592/@@600/@@602/@@606/@@610/@@611/@@612/@@628/@@629/@@638/@@639/@@6092/@@5929/@@5930/@@5931/@@5932/@@5933/FantasyCellar/BakedLightmap").)
   at: get_node (scene/main/node.cpp:1322)
ERROR: Condition "!vi" is true. Continuing.
   at: _clear_lightmaps (scene/3d/baked_lightmap.cpp:1195)

Note that to properly reproduce it one needs to reimport materials (deleting .import/ is the safest way to do it).

Edit: Seems related to glTF import, so deleting .import/*.glb-* seems sufficient before testing a new version to see if the import is bogus.

[akien-mga]

I bisected the regression to 6ec9468 (#49120), CC @lyuma @fire.

$ git bisect log
git bisect start
# good: [a69cc9f13da50ddf18452cfac2a5755624e1e971] Upgrade Embree to the latest official release.
git bisect good a69cc9f13da50ddf18452cfac2a5755624e1e971
# bad: [207fb165bfd1fefd1b4339c9427a569b19d0dcae] [HTML5] Raise default initial memory to 32 MiB.
git bisect bad 207fb165bfd1fefd1b4339c9427a569b19d0dcae
# bad: [28aacbfe85bc64b9639b0c0f25c0d84db24f6db8] Allow higher and lower maximum zoom values in GraphEdit
git bisect bad 28aacbfe85bc64b9639b0c0f25c0d84db24f6db8
# bad: [0b8e079eec322baf4d9c4a4d30c34c53937a46bd] New icons for Gradient and GradientTexture resources
git bisect bad 0b8e079eec322baf4d9c4a4d30c34c53937a46bd
# good: [8326b8b31a3d36dfa81417dfab6a6debc970052a] Merge pull request #49195 from madmiraal/fix-43544-3.x
git bisect good 8326b8b31a3d36dfa81417dfab6a6debc970052a
# bad: [e0fb05ad3063df9d9e5977480f333f635d691310] Merge pull request #49237 from akien-mga/3.x-cherrypicks
git bisect bad e0fb05ad3063df9d9e5977480f333f635d691310
# bad: [93d157d21353f6cd56af2912132e6ffcf3490111] Tweak Camera2D editor line colors for better visibility
git bisect bad 93d157d21353f6cd56af2912132e6ffcf3490111
# bad: [21eea9cd6c90cbbfd8c0551cf3c1f4083fa44c44] Make GraphNode handle children with EXPAND flag
git bisect bad 21eea9cd6c90cbbfd8c0551cf3c1f4083fa44c44
# bad: [9b35708a2145b5dc329aa0a91858b56ed27b462a] Merge pull request #49120 from lyuma/gltf-module-3.x
git bisect bad 9b35708a2145b5dc329aa0a91858b56ed27b462a
# bad: [d699600ec71bbf7733415bcd90ffd4707ed27792] gltf: Fix mesh nodes which are also bones.
git bisect bad d699600ec71bbf7733415bcd90ffd4707ed27792
# bad: [6ec9468e75e88e03c2ea237194e0ddfda94bf24a] Backport gltf2 module from master.
git bisect bad 6ec9468e75e88e03c2ea237194e0ddfda94bf24a
# first bad commit: [6ec9468e75e88e03c2ea237194e0ddfda94bf24a] Backport gltf2 module from master.

(For the reference 6ec9468 doesn't build... There's a stray use_xform in gltf_document.cpp which needs to be removed manually for the compilation to proceed.)

[SaracenOne]

I have debugged this and I think I can provide a highly likely explanation for this regression. The line in question is line 603

state->scene_name = _gen_unique_name(state, state->filename);

which results in a naming conflict since the node containing the meshes end up with the same name as root node and will increment the name by appending a number to it. This subsequently breaks the material assignment paths in the inherited scenes set up in this project.

The original GLTF importer from 3.3 used this line instead:

state.scene_name = _gen_unique_name(state, "Scene");

We can revert to the old behaviour, but that could introduce naming conflicts of its own, and I'd be curious if anyone knows the reasoning behind the change. Any feedback is appreciated.

[SaracenOne]

Okay, have a fix for this, but in the interest of not making PRs when I'm tired, I'll sleep on it first. The basic fix here is to not generate a unique scene name at first, simply assign it directly, but then run it through _gen_unique_name at the end of the _assign_scene_names method. This will ensure that the scene_name will always be assigned last and end up with incremented name. Since Godot's general scene importer supercedes the root name anyway, this would make the naming conflict irrelevant. However, there still questions which may need to be addressed: how do we have consistent GLTF Import/Export with a duplicate root node name and is it possible that we could run into other naming conflicts with other empty-named nodes.

[akien-mga]

Fixed by #53140.