Fix custom tracks causing issues on reimport

[lordkettune]

Upon importing a scene with custom tracks enabled, the importer was treating all of the tracks as custom tracks, meaning that when reimported it would overwrite the tracks with the previously imported ones. I just deleted the bit of code that did that.

Also fixed an issue with copy_track in which it would try to copy a value track's update mode even if it wasn't a value track, which led to it printing a ton of error messages each time you reimported a scene.

(This is my first time contributing here by the way, let me know if I'm doing anything wrong)

Bugsquad edit: Fixes #39959.

[aaronfranke]

See also #39657, cc @Sl3dge78 @reduz.

[lordkettune]

That PR must've been what broke it. I think they were calling anim->set_imported_track(i, false) to fix the warning on the animation editor displaying when it shouldn't be, which is also annoying, but it broke the importer as a result. It should be doing the exact opposite if anything, which it looks like it was before that PR.

[Sl3dge78]

Hey thank you for cc ing me and sorry if my PR caused any issues...

Just to understand your issue, you imported a scene with animation to external files, modified existing tracks and/or added new ones, and finally saved said animation. Then when pressing reimport what happened?

Here's what I just tested on my side and what happens in both cases :

• Import a .dae file with animation and store the animation outside the dae
• Edit an animation (by changing tracks or adding new ones) and save it
• Reimport the .dae with the Keep Custom Tracks enabled.

old behaviour (before my pr):

• Edited tracks are reverted
• Created tracks are kept

new behaviour (with my pr):

• It duplicates all tracks (that's a big issue!)

This is where the merge happens :

godot/editor/import/resource_importer_scene.cpp

Lines 959 to 971 in 73262c0

	if (FileAccess::exists(ext_name) && p_keep_animations) {
	//try to keep custom animation tracks
	Ref<Animation> old_anim = ResourceLoader::load(ext_name, "Animation", true);
	if (old_anim.is_valid()) {
	//meergeee
	for (int i = 0; i < old_anim->get_track_count(); i++) {
	if (!old_anim->track_is_imported(i)) {
	old_anim->copy_track(i, anim);
	}
	}
	anim->set_loop(old_anim->has_loop());
	}
	}

The snippet above adds any track that isn't marked as imported from the old file to the new file, so that's why.

Maybe the operation shouldn't be a copy but a replace? If the track exists, replace it if its not imported.
If the user wants to reverts the changes he has made he can untick "keep custom tracks"

Can you confirm that this is the behaviour you were having?

[lordkettune]

Yes, that's the issue I was having.

I don't think it should replace the tracks either, if it did you would have to lose your custom tracks every time you want to change your animations, which defeats the purpose.

I believe that calling anim->set_imported_track(i, true) on all of the tracks that came from the source file was the correct solution. I'm going to add that back in once I'm at home and I can update this PR.

[lordkettune]

Fixes #39959

[Sl3dge78]

Thank you for the change and sorry again for the trouble.
Edited tracks will be reverted on reimport, but to fix that we'd need to be able to compare tracks maybe via their paths. That's probably too big of an edge case.

[lordkettune]

Don't be sorry, it's no trouble!
I think that's the expected behavior - edited tracks should be reverted on reimport, the user should be able to edit their animations, then reimport the file, and it should update the imported tracks with the changes they made but not delete the custom tracks they added.

[CowKeyMan]

Don't be sorry, it's no trouble!
I think that's the expected behavior - edited tracks should be reverted on reimport, the user should be able to edit their animations, then reimport the file, and it should update the imported tracks with the changes they made but not delete the custom tracks they added.

As someone doing a lot of reiterating and reimporting of 3D models with animations, I agree with this 100%. Thank you to the both of you. Hope this gets merged soon ^-^

I'm still a bit new here, should I close this issue: #39959 ? Or should I wait until it gets merged into master?

[lordkettune]

I tweaked this a little and it should be ready now.

@CowKeyMan I think wait until it gets merged into master.

I'm also wondering if there's a good way to hide the imported tracks warning when keep_custom_tracks is enabled. It's a little misleading when it displays and the user already has that enabled. Does anyone know if there's a way to check a resource's import settings through the editor?

If not the only way I can think of is storing that flag from the importer in each animation, but I don't think that's a very elegant way to handle it.

[CowKeyMan]

I tweaked this a little and it should be ready now.

@CowKeyMan I think wait until it gets merged into master.

I'm also wondering if there's a good way to hide the imported tracks warning when keep_custom_tracks is enabled. It's a little misleading when it displays and the user already has that enabled. Does anyone know if there's a way to check a resource's import settings through the editor?

If not the only way I can think of is storing that flag from the importer in each animation, but I don't think that's a very elegant way to handle it.

There's also sometimes another weird bug that appears, where if you reimport while godot is open, you either need restart godot or else double click the .glb/.gltf file (havent tried it with others yet) and open 'new inherited scene' before the new glb/gltf file actually takes effect. During this state, if you try to create new custom animation tracks or edit previous one, the changes end up getting lost. I'll put a more detailed description of this in an issue soon, gonna check if it still happens in 3.2.2 after this merge. That said, thank you very much once again for the fix!

Note: the bug Im referring to was in 3.2.1 as well, in case that wasn't clear

[lordkettune]

@CowKeyMan No problem. I'm hoping this can be backported, the issue is the same on 3.2.

[aaronfranke]

Since these changes are small, this could probably be squashed down to 1 commit.

[akien-mga]

Thanks! And congrats for your first merged PR :)

[akien-mga]

Cherry-picked for 3.2.3.