Added signals to AnimationNodeStateMachine #30508
Conversation
|
Again: it'd be nice to have more descriptive signal names for this than |
| float AnimationNodeStateMachine::process(float p_time, bool p_seek) { | ||
|
|
||
| Ref<AnimationNodeStateMachinePlayback> playback = get_parameter(this->playback); | ||
| ERR_FAIL_COND_V(playback.is_null(), 0.0); | ||
|
|
||
| if (!playback->is_connected("state_changed", this, "_on_playback_state_changed")) { |
There was a problem hiding this comment.
I wonder whether it's possible to simply connect the state_changed signal whenever AnimationNodeStateMachinePlayback is assigned to state machine rather than checking this in process, though I don't see an easy way to assign it here. Perhaps it should be handled somewhere in _set('parameters/playback', playback).
AnimationNodeStateMachinePlayback is also not shared between instances by default, so I wonder whether it's by design and if it makes sense to create and assign a different playback resource at all:
AnimationNodeStateMachinePlayback::AnimationNodeStateMachinePlayback() {
set_local_to_scene(true); //only one per instanced scene
// ...
}There was a problem hiding this comment.
This seems to be the only place that instances playback resource:
Variant AnimationNodeStateMachine::get_parameter_default_value(const StringName &p_parameter) const {
if (p_parameter == playback) {
Ref<AnimationNodeStateMachinePlayback> p;
p.instance();
// Connect here?
// p->connect("state_changed", this, "_on_playback_state_changed"));
return p;
} else {
return false; //advance condition
}
}There was a problem hiding this comment.
Ok I'll try a version where it connects here and run my tests to see
There was a problem hiding this comment.
@Xrayez @jedStevens I've got a working version where the connection is made in AnimationNodeStateMachine::get_parameter_default_value() and also in AnimationNodeStateMachine::_set(). Note that this requires AnimationNodeStateMachine::get_parameter_default_value() to no longer be a const method.
Is it possible for me to push my changes to this PR or do I need to open a new one?
|
@Sslaxx I was just taking a look and I'm curious as to which signals you might have been hinting at. My simple test seems to work with this current implementation. I'm unsure of what you meant specifically was all. |
|
Perhaps "more descriptive" means answering the question of whether the signal is fired before the actual state is about to be changed, or is it actually changed already. But that should be reflected in documentation if that's the case. |
|
I suppose since it is the line just before the actual assignment, this could cause errors. I can modify it so that it temporarily stores the previous state and emit the signal after the assignment to not cause any confusion about if it's been changed or not. |
|
@jedStevens Is this still desired? If so, it needs to be rebased on the latest master branch. |
|
The repository this PR comes from has been deleted, so this PR is no longer valid. Closing. |
Sorry for re-opening this PR but I royally screwed the last one (#24328) so please forgive me; it's been recently closed in favor of this PR.
This should expose the
from_stateandto_statewhen an AnimationNodeStateMachine changes states. This should allow for a more direct control with AnimationTrees; by setting "advance parameters" in the tree, there can be a response when the signal is emitted.I welcome any additional testing but this works on my machine quite well and I can't seem to find errors. Let me know if you can.