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

Allow keying properties when selecting multiple nodes #92842

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Jun 6, 2024

MultiTrackDrifting.mp4

Currently it's not possible to key properties for animation tracks when having multiple nodes selected in the scene tree, as pointed out among the many quality-of-life improvements in this proposal (as "Add several tracks/keyframes when several nodes are selected").

This seemed fairly straight-forward to address, by simply allowing the keying to not only work on Node but also on MultiNodeEdit, which is exactly what this PR does.

One thing that's perhaps up for debate in terms of UX is what value should be put into the keys when you have multiple nodes selected, given that MultiNodeEdit will display the value from the first selected node, even if the values of the multiple nodes differ. It seemed to me that the most intuitive implementation was to have each node's respective value entered into their respective keys, rather than the value of the first selected node be entered into all nodes' keys. For this reason the p_value passed into AnimationPlayerEditorPlugin::_property_keyed is now ignored and fetched from each respective node instead.

(I would perhaps also argue that MultiNodeEdit shouldn't work this way to begin with, and should render the value/control in some other way when the values differ between the nodes, which I believe is how Unreal does it, and perhaps other editors as well, but that's a separate discussion.)

In the process of doing this I felt the need to create a new method in AnimationTrackEditor that would be similar to insert_value_key but which accepted a specific Node instead and found that there was already such a method, called insert_node_value_key, so I made use of that instead (with some tweaks). The only major difference between these that I could spot was the inclusion of make_insert_queue and commit_insert_queue in the old insert_value_key, which I left in the now refactored insert_value_key. I welcome feedback as to whether this approach might have some unintended consequences.

@mihe
Copy link
Contributor Author

mihe commented Jun 7, 2024

I figured it made sense to have insert_node_value_key and the now refactored insert_value_key be consistent in terms of not taking a value but instead just fetching the value based on the property name passed in, so I did just that. It'll be a tad bit slower obviously, but it's not a hot path anyway, and this new interface should leave less room for shooting yourself in the foot by mismatching the property name and value.

@mihe mihe force-pushed the multi-node-keying branch 3 times, most recently from cf12f5e to 13fc7df Compare June 7, 2024 10:15
@akien-mga akien-mga requested a review from KoBeWi August 9, 2024 08:56
@mihe mihe force-pushed the multi-node-keying branch from 13fc7df to 51fca65 Compare August 19, 2024 11:51
@mihe mihe force-pushed the multi-node-keying branch from 51fca65 to 351f454 Compare August 19, 2024 12:05
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Works correctly.

Could use a look from @TokageItLab

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Aug 20, 2024
@TokageItLab TokageItLab self-requested a review August 26, 2024 06:47
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

Seems to be working well.

@akien-mga akien-mga merged commit b310e5e into godotengine:master Aug 30, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@mihe mihe deleted the multi-node-keying branch August 30, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants