Fix re-import glb model doesn't change the old glb model

[Hilderin]

• Fixes Re-import glb model doesn't change the old glb model #90241
• Fixes glTF files aren't always automatically reimported #90659
• Fixes [glTF] Wrong object mesh when two objects have a same name without an import hint #92837

It took me a while to figure this out, but finally, the fix seems easy.

The reason why the model was not updated live in the scene is that the method EditorNode::get_modified_properties_for_node was returning all the properties of the nodes, like the mesh, when an autoreload had been done at least once. The behavior in place is to overwrite the properties modified by the user with the values in the current scene. This behavior ensures that if a user changes a property manually, the reimportation does not overwrite these properties. However, because Godot thought all the properties were modified by the user, when the new model was loaded, the old mesh property was copied over the new one.

Why did EditorNode::get_modified_properties_for_node return properties that were not modified? Because the instance_state was set to null in reload_instances_with_path_in_edited_scenes for an unknown reason. Since instance_state was not set, the method PropertyUtils::get_property_default_value was not able to find the source scene, and therefore the default value of the properties.

Before the modification:

godot.windows.editor.x86_64_bnnPCN1dRq.mp4

After the modification:

godot.windows.editor.dev.x86_64.mono_b5WHMDxMIx.mp4

MRP to test:
Test Reimport Blender.zip

EDIT: Add fixes for #90659 and #92837

Bugsquad edit to properly use closing keywords.

[akien-mga]

CC @passivestar

[passivestar]

Tested for a bit, this fixes both #90241 and #90659 and some other issues that I didn't report, apparently they were caused by this

This makes it possible to use blender for level design. When the fix for the error message spam (#93765) also gets merged it will make the whole process work almost perfectly. The only notable problem left to solve will be name conflicts with hints (#92837) and the likes, but those are nowhere near as important as what this PR fixes

Thanks! 🙏

[akien-mga]

@SaracenOne added the lines in #92279, would be good to review why. And worth noting that it means this fixes the issue in 4.3 but it's also reported in 4.2 which didn't have #92279, so the problem there was likely different (maybe #92279 was a necessary part of fixing this).

[Hilderin]

I look a bit at the PR #92279 and effectively, these modifications seems necessary to make the hot reload possible and that's why it probably did not work in 4.2 either.

[Hilderin]

I tested #92837 and I'm able to reproduce the problem with master branch but not with the modification from this PR. I guess this PR also fixes #92837 and it's probably because it does not overwrite mesh values anymore.

Master branch:

godot.windows.editor.dev.x86_64.mono_KqMt9Ope0w.mp4

With this PR:

godot.windows.editor.dev.x86_64.mono_47Rw8KIWVO.mp4

[passivestar]

I guess this PR also fixes #92837

Hm you're right, I just tested it and can't reproduce it either. Great news!

One related problem that I noticed (haven't reported yet) is that if you have an object called Cube-col, add some children to it in godot and then add an object called Cube in blender and reimport, Cube will steal the children of Cube-col :)

Edit: Issue opened - #94031

[Hilderin]

One related problem that I noticed (haven't reported yet) is that if you have an object called Cube-col, add some children to it in godot and then add an object called Cube in blender and reimport, Cube will steal the children of Cube-col :)

When the issue is created, maybe send me this issue number, I could look into it when I have the time.

[fire]

I'll ask around the original reasoning for the line, but it seems to resolve multiple people's issues.

Edited:

Conversation mentioned it might be related to the bug where node references needed to reacquire the node from their paths.

[SaracenOne]

As you've not probably seen, the system for hot reloading assets turned into a bit of complexity explosion. I tried to tame it the best I could, but there was really only so much I could do.

This bug seems to have passed by my regular testing suite due to the fact that it really only appears to manifest after multiple reimports of the same scene which it doesn't look like I properly covered for (plus the fact that it wasn't really modifying models, just reimporting them in the process of trying to track down crashes). I also don't entirely remember the exact reason I cleared the scene state (and comment reads like I was banging my head on the desk mid way through writing it), but it seems to have something to do with what the last PR I did was meant to fix, namely that the previous iteration of the reimport system didn't recache direct node script references (a relatively new feature in the engine), which would lead to the old freed ones being copied over which would result in hard engine crashes.

I'm not sure why clearing the scene state helps with this, though it might have something to do with scene states holding a HashMap of nodepaths, but given that these would get reset anyway, I'm not sure. There is perfectly reasonable probability that simply removing this line will break another feature of the system in some obscure way, but I've yet to find anything, so if I can't anything by the end of the day, I'll approve this and we'll just hope for the best, since the current behaviour is quite clearly still incorrect one way or another.

[SaracenOne]

I think its fine to give this fix a try given I can't find anything else that specifically breaks after removing it. It's still possible a regression in another area might still pop up, but evidently this does fix unambigious bugs and will make the feature more immediately useful to people.

[akien-mga]

Thanks! Great finding 🎉