Skip to content

Generate value blocks for AnimationPlayer animations #191

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

Closed
wants to merge 3 commits into from

Conversation

dylanmccall
Copy link
Contributor

It is possible for the list of animations to change based on changes
done in another panel. This causes the list of animation value blocks in
the Block Code panel to become stale. Ideally we should watch the
mixer_updated signal in an AnimationPlayer node, but at the moment this
would be difficult to implement. Watching for the panel being shown will
work around the issue for a reasonable number of situations.

https://phabricator.endlessm.com/T35564
Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

This looks great, only a nit comment. I think it's fine to refresh the animations every time the panel is shown, as the last commit does. Maybe later we can add a refresh button in the title bar.

I wonder if we should join forces and land #147 first.

@@ -55,7 +55,7 @@ func _on_undo_redo_version_changed():
return

var block_script: BlockScriptSerialization = _current_block_code_node.block_script
_picker.block_script_selected(block_script)
_picker.block_script_selected(block_script, _current_block_code_node.get_parent() if _current_block_code_node else null)
Copy link
Contributor

Choose a reason for hiding this comment

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

The line above is assuming that _current_block_code_node is not null (it is referencing block_script in it.

In any case, this would be a bit more readable without a ternary operator.

@@ -117,7 +117,7 @@ func switch_block_code_node(block_code_node: BlockCode):
_delete_node_button.disabled = _current_block_code_node == null
if _current_block_code_node != null:
_try_migration()
_picker.block_script_selected(block_script)
_picker.block_script_selected(block_script, _current_block_code_node.get_parent() if _current_block_code_node else null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this would be a bit more readable without a ternary operator. Or at least moving the conditional to a variable:

var parent_node: Node = _current_block_code_node.get_parent() if _current_block_code_node else null
_picker.block_script_selected(block_script, parent_node)

Comment on lines 631 to 633
static func get_blocks_for_object(object: Object) -> Array[Block]:
var blocks: Array[Block] = []
return blocks
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@manuq manuq left a comment

Choose a reason for hiding this comment

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

Sorry for the double review. Now I gave it a try and it's not working. The generated script is missing the quotes around the aniimation name:
Grabación de pantalla desde 2024-08-12 15-22-52.webm

This makes me wonder if the animation values should be options in a dropdown of the Play block. Otherwise these are string blocks that can be used anywhere else, but are only relevant for playing the animation.

@dylanmccall
Copy link
Contributor Author

This makes me wonder if the animation values should be options in a dropdown of the Play block. Otherwise these are string blocks that can be used anywhere else, but are only relevant for playing the animation.

Yeah, I'll give that a try :) In that case it'll definitely need to be on top of #147, and might need some new pieces? But I'll see where I can get it!

@dbnicholson
Copy link
Member

@dylanmccall I believe this all got reimplemented in #206, correct? Can we close this?

@dylanmccall
Copy link
Contributor Author

Yes! Thanks for remembering.

@dylanmccall dylanmccall deleted the T35564-animation-value-blocks branch September 24, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants