Fix import hints breaking node paths in animations

[rburing]

Keep track of renamed nodes (which are processed first) so that in the animations (which are processed last) the paths can be fixed.

Fixes #38809. Importing the minimal example there now gives the following verbose output:

Fix: Renamed Marker-col to Marker
Fix: Correcting node path in animation track: Marker-col should be Marker

Small bonus: fixes the unreported bug that the convcolonly hint was not removed in case of an empty object with a draw type.

Bugsquad Edit:

• Fixes Importer doesn't drop -col and -colonly suffixes in animation tracks, but removes them from node names #42041
• Fixes Exported animations in glTF 2 from Blender don't works in Godot (for parented object and object with argument like -col) #38163

[fire]

Slow to get to this, putting it on my todo list.

[fire]

https://github.com/godotengine/godot-tests/tree/master/tests/blend_export/blend

Can you make a test case pr with the new tests?

Edited: reviewing

[rburing]

To clarify the (small) scope of this PR, let me note that the code that deals with empties is barely touched here (only fixing one renaming oversight). There remains a separate issue with them, namely importing them as custom collision shapes (based on the draw type) only works with files exported from Blender using ol' Better Collada: godotengine/godot-docs#4374

[fire]

I added 43044-marker.blend to https://github.com/godotengine/godot-tests/tree/master/tests/blend_export .

Currently building the engine to test that it breaks in 4.0 standard and works in your pr.

Edited link.

[fire]

I couldn't see a difference, maybe I'm misunderstanding how this works. Notice I have a mesh called marker next to marker-col.

This PR.

Master.

[rburing]

The original marker.blend file in #38809 contains only Marker-col; the 43044-marker.blend test file contains a second Marker in addition to Marker-col. This reveals the following (hidden?) functionality:

godot/editor/import/resource_importer_scene.cpp

Lines 514 to 515 in c56a071

	if (mi->get_parent() && !mi->get_parent()->has_node(fixed_name)) {
	mi->set_name(fixed_name);

Namely, the -col suffix is not removed if it clashes with an existing name. In that case there is no need to fix animations, so the animations from the 43044-marker.blend work even on master without modifications (and in my PR, the new code is simply not triggered, and so it also works). When there is no clash with an existing name (as will be the common case, and as it was in #38809), there will be a rename that removes the -col suffix, and the code in my PR will be triggered, fixing the animations.

So, 43044-marker.blend currently does not test the code in this PR (the original marker.blend does).

[fire]

I wonder why Marker-col and not Marker2? That seems odd.

[rburing]

The quoted if statement completely prevents the rename (in case that there would be a clash), so in that sense it is normal.

If Marker2 is preferred then the logic should be changed.

[reduz]

I will be reworking this code in the coming weeks, can probably use it as a reference but suggest review for 3.2

[fire]

What is the current state of this? Need some help to retest master.

[reduz]

This looks good, but likely needs a rebase. Any volunteers?

[rburing]

Thanks for the feedback. I'll rebase today.

[rburing]

Rebased it is.

[rburing]

Originally this PR was doing the renaming work in the same loop as the one where tracks are being removed if they reference nodes with a noimp (no import) hint. That original version would have introduced a bug if an animation would have a track referencing a node with a noimp hint and a track referencing a node with an import hint that would trigger a rename. In the latest version the removing and renaming are done in two separate loops (first removing, then renaming), for simplicity and safety.

[rburing]

I added a local variable bool nodes_were_renamed = r_node_renames.size() != 0, so that we try to fix node paths (which involves iterating over all tracks of all animations) only when nodes_were_renamed.

If the original PR looked good and the (small) improvements are agreeable, then does it mean a positive review? :)

[akien-mga]

Thanks!

[akien-mga]

This might warrant a dedicated backport PR for 3.x if needed there, as the change is not trivial to cherry-pick.