Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type filters to AnimationPlayer's "Add Track" #98558

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

jasonmorgado
Copy link
Contributor

@jasonmorgado jasonmorgado commented Oct 26, 2024

(My first contribution btw)

Adds type filters to the SceneTreeDialogs that come from AnimationPlayer's "Add Track" button.
The Add Track button has several types of tracks that require specific node types, but currently present a full SceneTreeDialog, and throw an error if you provide the wrong type.

Fixes #55944 which describes this issue specifically for Audio tracks.

image

The SceneTreeDialog that appears when Add Track > Audio Playback Track is selected. Now with the correct filters!

  • Property, Call Method, Bezier Curve have no filters
  • Blend Shape has a Meshinstance3D filter
  • 3D Position, Rotation, Scale tracks have a Node3D filter
  • Audio has AudioStreamPlayer(2D/3D) filter
  • Animation has AnimationPlayer filter

I've tested the SceneTreeDialog in the inspector (export nodepath), works the same as usual. However, I'm not sure where else to find the prompts for the SceneTreeDialog in the editor for further testing.
I see it's in a few different places in the code such as:

  • EditorInterface
  • EditorPropertyNodePath (The one I tested)
  • MultiMeshEditor
  • Particles3DEditorPlugin
  • ReplicationEditor
    All these examples set filters once after creating the dialog, so they should be unaffected.

Related, I did notice the SceneTreeDialog's set_valid_types was not set up correctly to be called on the same instance of SceneTreeDialog multiple times.
Seems the current workaround was deleting and recreating it.
See also this comment:

// TODO: Should reuse dialog instance instead of creating a fresh one, but need to rework set_valid_types first.
if (node_selector) {
node_selector->disconnect(SNAME("selected"), callable_mp(this, &EditorInterface::_node_selected).bind(p_callback));
node_selector->disconnect(SNAME("canceled"), callable_mp(this, &EditorInterface::_node_selection_canceled).bind(p_callback));
get_base_control()->remove_child(node_selector);
node_selector->queue_free();
}
node_selector = memnew(SceneTreeDialog);

I took a stab at fixing this issue, clearing the filter's label and types before setting it.
Fixed the icons not showing up, but it calls _notification() directly to update the icon sizes. Would like some feedback on that too, seems unusual for the codebase.

@jasonmorgado
Copy link
Contributor Author

After further testing I've found a bug where an error is thrown when alternating between adding Audio Tracks and Property tracks. (Between a type with and without a filter, needs to switch a few times).
I'll dig into it further, but if it doesn't work I can reuse the existing workaround of just re-creating the dialog box. Maybe write up an issue for the strange behavior around set_valid_types().

@jasonmorgado jasonmorgado force-pushed the add-track-filter branch 3 times, most recently from f90bac8 to 3da5b76 Compare November 2, 2024 17:02
@jasonmorgado
Copy link
Contributor Author

I've fixed the more consistent error message, but I occasionally get this one:
core/object/object.h:1036 - Condition "slot >= slot_max" is true. Returning: nullptr
So far I've only been able to replicate it quickly toggling between a Property and a Property with a filter, like Position 3D, opening the SceneTreeDialog, closing it with Esc, and opening a different one.

I guess if you don't spam open/close the Add Track dialog it should be fine. I'm not sure what causes this particular error message, so I could use some feedback if it would prevent the PR from being merged or can be ignored.

@jasonmorgado jasonmorgado force-pushed the add-track-filter branch 2 times, most recently from f1454d2 to c542462 Compare November 3, 2024 13:42
@fire fire requested a review from TokageItLab November 4, 2024 20:55
@KoBeWi KoBeWi modified the milestones: 4.x, 4.4 Nov 6, 2024
Comment on lines 5303 to 5318
// Blend Shape is a property of MeshInstance3D
valid_types.push_back(StringName("MeshInstance3D"));
} break;
case Animation::TYPE_POSITION_3D:
case Animation::TYPE_ROTATION_3D:
case Animation::TYPE_SCALE_3D: {
// 3D Properties come from nodes inheriting Node3D
valid_types.push_back(StringName("Node3D"));
} break;
case Animation::TYPE_AUDIO: {
valid_types.push_back(StringName("AudioStreamPlayer"));
valid_types.push_back(StringName("AudioStreamPlayer2D"));
valid_types.push_back(StringName("AudioStreamPlayer3D"));
} break;
case Animation::TYPE_ANIMATION: {
valid_types.push_back(StringName("AnimationPlayer"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Blend Shape is a property of MeshInstance3D
valid_types.push_back(StringName("MeshInstance3D"));
} break;
case Animation::TYPE_POSITION_3D:
case Animation::TYPE_ROTATION_3D:
case Animation::TYPE_SCALE_3D: {
// 3D Properties come from nodes inheriting Node3D
valid_types.push_back(StringName("Node3D"));
} break;
case Animation::TYPE_AUDIO: {
valid_types.push_back(StringName("AudioStreamPlayer"));
valid_types.push_back(StringName("AudioStreamPlayer2D"));
valid_types.push_back(StringName("AudioStreamPlayer3D"));
} break;
case Animation::TYPE_ANIMATION: {
valid_types.push_back(StringName("AnimationPlayer"));
// Blend Shape is a property of MeshInstance3D.
valid_types.push_back(SNAME("MeshInstance3D"));
} break;
case Animation::TYPE_POSITION_3D:
case Animation::TYPE_ROTATION_3D:
case Animation::TYPE_SCALE_3D: {
// 3D Properties come from nodes inheriting Node3D.
valid_types.push_back(SNAME("Node3D"));
} break;
case Animation::TYPE_AUDIO: {
valid_types.push_back(SNAME("AudioStreamPlayer"));
valid_types.push_back(SNAME("AudioStreamPlayer2D"));
valid_types.push_back(SNAME("AudioStreamPlayer3D"));
} break;
case Animation::TYPE_ANIMATION: {
valid_types.push_back(SNAME("AnimationPlayer"));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've committed the change here. Is there a particular reasoning behind using SNAME vs StringName? or even SceneStringName? Or plain string with just quotes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a dedicated macro creating a static instance, otherwise this creates a new StringName instance each time this method is run, forgot to add that to the comment when remaking it after some other changes

@jasonmorgado jasonmorgado force-pushed the add-track-filter branch 2 times, most recently from d472737 to 8c46c00 Compare November 9, 2024 15:12
Co-Authored-By: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Co-Authored-By: Tomek <kobewi4e@gmail.com>
@Repiteo Repiteo merged commit 0f5f3bc into godotengine:master Nov 10, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 10, 2024

Thanks! Congratulations on your first contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Picking node for audio track could be improved
4 participants