Improve Responder Workflow#456
Open
Hoikas wants to merge 7 commits into
Open
Conversation
In my experience, Responders generally have only a single state and fire on trigger. It's really just a means to fire some messages, sometimes with waits in the middle. That means the artist has quite a bit of overhead that they don't need. Enter the basic Responder. It links to a condition that fires it and directly sends messages. Under the hood, it creates the single state at export time and only fires on "trigger" (state==1.0). This should make life easier moving forward.
This adds the "Notify Triggerer" node and allows the artist to manually choose when to notify a Python script. This matches the UX of PlasmaMax and also allows exposing the event ID that will be returned to the triggerer. I actually used this ability when fixing the BlueSpiral doors in Eder Delin and Eder Tsogal, but that was done manually. Previously, the artist had no control over when the notification was sent - always at the end of the Responder. Further, there was a bug in that the event ID was left at 0 instead of the expected default of 1. The old behavior will be retained on old Responders with each Responder state node gaining a checkbox to enable/disable the feature. Old nodes will default to having the "auto notify" turned on if the Responder is linked to a PFM. If the user attaches a "Notify Triggerer" and links it to a state, auto notifies are disabled for that Responder state. Newly created nodes default the "auto notify" feature to off. This is a good time to make this change because I'm able to add these backwards compatibility mitigations *now*. The new basic Responder node does not link to states, so it will *never* send auto notifies.
This should be safe. When the Responder code was originally written, there were quite a few logic errors whereby we could get stuck waiting on things or messages would be sent out of order. Since then, I've tightened the logic up, and, in theory, only one branch of a Responder tree should be able to wait. So, overriding the `waitOn` local should be fine - there was no need to retain the previous value from what I could tell. Having a secondary `childWaitOn` opened us up to an `UnboundLocalError` when a callback message did not have any linked callbacks. I'm kind of surprised we never got any bug reports about this.
This improves the text for the PFM reference socket and the "detect (un)trigger" checkboxes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves the Responder node workflow by adding a new node - the Basic Responder. The basic Responder allows artists to more simply graph trivial, single state Responders with fewer nodes. The Basic Responder node accepts a condition, a PFM reference, and a list of messages to send.
While I was here, I added the ability to explicitly trigger the referenced PFM using a new Notify Triggerer message node, which matches the behavior of PlasmaMax. The old "automatically notify any referenced PFMs when a state completes" is now an opt-in behavior exposed on the Responder State node. Existing Responder State nodes attached to Responder nodes with PFMs attached will default to this being ON. Newly created Responder State nodes will default to this being OFF. Basic Responders will require manual additions of Notify Trigger message nodes. I also fixed the auto notify messages to send the eventType of 1 instead of 0, matching the behavior of PlasmaMax.
An illustration of the changes:
I'm going to leave this open for feedback for a time, then merge. I have a set of SDL helper nodes that I'm curating that will need to be slightly updated for basic responders, so I'd like to go ahead and get this in.