AnnotationsGadget : Add {plug} syntax for substituting node values
#5901
Conversation
johnhaddon
force-pushed
the
annotationSubstitutions
branch
from
4ec5930
to
df0a529
Compare
|
Pushed a rebase to fix the merge conflict and the build on Windows. |
There was a problem hiding this comment.
Thanks John! This looks super useful. I've tried my best to horribly break things, but no luck so far. :)
There's a comment inline about the completion menu behaviour. The first paragraph is worth addressing, but see how you feel on the second.
One other behaviour noticed while trying to break things, deleting a plug referred to by an annotation leaves the last value around until the annotation is updated by some other means. Might be a hassle to track the plug removals, so see how you feel about it. I'm happy to leave it be if it's too much bother.
Something that could be worth considering as a later PR is adding the ability to create annotations from the plugs themselves via the Node Editor plug context menu. So you could right-click on a plug and "Annotate in Graph Editor" to create/toggle an annotation for that plug. I'm not entirely sold on the idea as it may be a little too "spooky action at a distance" with the result of the action only directly apparent if the node is also visible in the Graph Editor. There'd also be considerations for whether those annotations would insert into the regular "user" annotation (Pros: Easier for the user to modify and may aid in discovery of the {plugName} syntax) or exist as individual plug:<plugName> annotation registrations (Pros: self-contained so they'd be simpler to toggle from the Node Editor context menu without fear of messing up the "user" annotation text, while also being separately filterable from "user" annotations in the Graph Editor Annotations menu.) Nothing to act on immediately, but I figure it's worth capturing the thought here for later discussion...
|
Thanks for the fixup! I think it'd be worth merging this to get a bit more testing in before a release. |
johnhaddon
force-pushed
the
annotationSubstitutions
branch
from
f4bc3ba
to
5cab540
Compare
I've squashed that fixup in now, and added one more commit which lists the available plugs in a right-click menu on the code widget. |
|
Thanks! The right-click menu definitely helps with plug discovery. It might be a little weird that we're including plugs in the menu that can't be displayed as annotations, but otherwise all looks good to me. |
This will facilitate testing as we add plug value substitutions in a future commit.
This is highly unlikely in practice at this very moment. But in an upcoming commit we're going to be doing some threading shenanigans that means that it is possible if a GraphEditor is destroyed while the AnnotationsGadget does work on a background thread. The code is also simplified a little by having an `annotations()` method compared to an error-prone `graphGadget()` method.
With the original code we were getting this compilation error : ``` C:\Users\John\dev\gaffer\src\GafferUI\AnnotationsGadget.cpp(694): error C2440: '<function-style-cast>': cannot convert from 'const GafferUI::AnnotationsGadget::schedulePlugValueSubstitutions::<lambda_865de2d9e4b3857c59238e0cbe33775b> *' to 'GafferUI::AnnotationsGadget::Ptr' ``` Looks like MSVC thought `this` was a pointer to the lambda and not to the AnnotationsGadget.
johnhaddon
force-pushed
the
annotationSubstitutions
branch
from
5cab540
to
854415c
Compare
Yeah, I was aware of that, but I didn't see much point doing the work of excluding them now, when the inevitable outcome either way is that I'll need to add more formatters when someone wants to use one anyway. Hopefully at that point I'll have some clearer ideas on how to share the formatting between Annotations/PathListingWidget/Spreadsheet...
Coolcool - thanks for all the testing and reviews. I'll merge now... |
This allows
{plug}syntax to be used to include the values of a Node's plugs in its annotations.A note on ABI : this does change the data members of AnnotationsGadget in a way which technically breaks ABI. But since AnnotationsGadget is only created by GraphGadget, and shouldn't be created or subclassed by anyone else, I can't see any way in which this can cause real-world problems. I've therefore made the PR to 1.4_maintenance to get it into people's hands sooner.