Numeric Bookmarks - Persistence #3157

mattigruener · 2019-05-11T00:55:32Z

Hey John,

I took a first stab at preserving the numeric bookmarks. Let's discuss some time if this is generally a good approach. The most contentious part is probably what I'm doing to serialise a special method that doesn't override existing numeric bookmarks.

There's probably no rush to get this done - I know you have plenty on your plate at the moment.

Cheerio,

Matti

mattigruener · 2019-05-13T18:55:11Z

Restarted failed MacOS test.

johnhaddon

Thanks Matti,

I've made a few comments inline, but I also want to make a counter-proposal to see how it strikes you. We've been gradually improving our serialisations so that they provide a better example of how you might use the Python API for real, so my proposal is simply that we continue in that direction : we serialise the bookmarks as a call to MetadataAlgo.setNumericBookmark(). We can then query Script::isExecuting() from setNumericBookmark() and use it to resolve clashes in any way we want.

To achieve that cleanly I think we'd add some sort of registerSerialiser( key, aFunctionThatReturnsASerialisation ) in MetadataBinding.h, and use it to register a custom serialiser for the "numericBookmark*" metadata. But since this is the first instance of such a requirement, I'm inclined to just hardcode the special case we need inside the implementation of metadataSerialisation(), and wait for a second use case to better inform any API design. How does that sound?

Cheers...
John

johnhaddon · 2019-05-14T13:38:37Z

src/Gaffer/MetadataAlgo.cpp

 	for( Node *nodeWithMetadata : Metadata::nodesWithMetadata( scriptNode, numericBookmarkMetadataName( bookmark ), /* instanceOnly = */ true ) )
 	{
+		// Numeric bookmarks brought in by References will be ignored
+		if( anyAncestorIs<Gaffer::Reference>( nodeWithMetadata ) )


Isn't this the same as if( nodeWithMetadata->ancestor<Reference>() )?

If we're going to take this "ignore bookmarks inside references" approach, then we need to update MetadataAlgo::numericBookmark( const Node * ) too. Currently, it is returning a bookmark for nodes inside references. This means you can load the metadata (provided it doesn't exist at the point the reference is loaded), and the bookmark annotation is shown in the GraphEditor. But then when you use the hotkey, nothing happens.

johnhaddon · 2019-05-14T14:14:50Z

include/Gaffer/MetadataAlgo.h

@@ -153,6 +154,10 @@ GAFFER_API void copy( const GraphComponent *from, GraphComponent *to, const IECo
 /// \undoable
 GAFFER_API void copyColors( const Gaffer::Plug *srcPlug, Gaffer::Plug *dstPlug, bool overwrite );

+/// Some metadata can cause clashes if respective entries exist in the scene already.
+GAFFER_API bool metadataRegistrationCanClash( const IECore::InternedString &key );


This doesn't feel quite right to me, but I'm struggling to rationalise/articulate why. I think a part of it is just the ambiguity of "can clash". What is a clash? Is it saying that it is allowed to clash, or that it isn't, or that it might and you need to deal with it?

But I think perhaps the bigger thing is that we already have special methods for registering numeric bookmarks, and now we're introducing another set of special methods that you need to use instead in certain circumstances. If we want to make the "non clashing metadata" concept watertight, then perhaps it would need to be a feature of Metadata rather than MetadataAlgo?

mattigruener · 2019-05-14T18:57:49Z

How does that sound?

Good - how do my updates look? :) I hope this is what you had in mind.

Thanks!

johnhaddon

Good - how do my updates look? :) I hope this is what you had in mind.

Thanks Matti - definitely what I had in mind. I've noted a couple of details that need addressing though. When you do those, could you just squash everything into one commit? It's not a lot of code, and the first three commits are totally reliant on each other...
Cheers...
John

johnhaddon · 2019-05-15T08:56:31Z

src/Gaffer/MetadataAlgo.cpp

@@ -245,7 +245,7 @@ void setNumericBookmark( ScriptNode *scriptNode, int bookmark, Node *node )
 		Metadata::deregisterValue( node, numericBookmarkMetadataName( currentValue) );
 	}

-	Metadata::registerValue( node, metadataName, new BoolData( true ), /* persistent = */ false );
+	Metadata::registerValue( node, metadataName, new BoolData( true ), /* persistent = */ true );


Let's squash this commit in with the ones that fix the serialisation - on its own it just creates problems.

johnhaddon · 2019-05-15T08:59:55Z

src/Gaffer/MetadataAlgo.cpp

@@ -230,6 +231,19 @@ void setNumericBookmark( ScriptNode *scriptNode, int bookmark, Node *node )
 	IECore::InternedString metadataName( numericBookmarkMetadataName( bookmark ) );
 	for( Node *nodeWithMetadata : Metadata::nodesWithMetadata( scriptNode, metadataName, /* instanceOnly = */ true ) )
 	{
+		// Numeric bookmarks brought in by References will be ignored


We can do better than this I think. If we put this check outside of the loop, then we will never load metadata inside references at all, clash or not :

if( scriptNode->isExecuting() && node && node->ancestor<Reference>() ) { // Never load numeric bookmarks inside references. return; }

I don't think joining the two checks is gonna work.

When in a Reference, we always need to block assigning numeric bookmarks. Not just when deserialising.

We need to block overriding numeric bookmarks outside of References via isExecuting()

I think what I've done now should take care of both of these and I've squashed it all down into just the one commit. Maybe have another look at it? :) Thanks!

I wasn't trying to suggest joining the "inside reference" and the "clashing during load" checks, just refining the reference check and leaving the "clashing during load" check where it was.

I was deliberately keeping the possibility of assigning non-serialisable bookmarks temporarily inside a Reference because I thought it might be useful when navigating big graphs, but maybe that was a bad idea. If we want to enforce the can't-bookmark-inside-a-reference behaviour then our convention is to do that via the UI rather than have the API silently fail :

https://github.com/GafferHQ/gaffer/blob/master/include/Gaffer/MetadataAlgo.h#L57-L81
https://github.com/GafferHQ/gaffer/blob/master/python/GafferUI/GraphBookmarksUI.py#L59

Sorry, I misunderstood.

I've just pushed the latest numeric bookmark (NB) code. Hopefully this is ok now.

added the check as you suggested outside of the loop -> never loads NBs in References.

kept the check inside the loop -> won't add NB when NB exists in graph already.

disallowed adding NB in References through the UI as per the file you pointed me to.

removed test that checks that the API silently fails when adding NB within Reference.

I've squashed it all into the one commit.

johnhaddon · 2019-05-15T09:04:42Z

src/Gaffer/MetadataAlgo.cpp

 	for( Node *nodeWithMetadata : Metadata::nodesWithMetadata( scriptNode, numericBookmarkMetadataName( bookmark ), /* instanceOnly = */ true ) )
 	{
+		// Numeric bookmarks brought in by References will be ignored


If we don't load numeric bookmarks into references (see suggestion above), then we wouldn't need to ignore them here, and we'd be properly enforcing our "there can be only one" rule, rather than just masking potential duplicates.

Ok, I guess I was trying to preserve the referenced data as much as possible and opted for ignoring instead of removing. Not loading the metadata is going to clean up the code a fair bit, though. Will push an update later today that'll address all of your feedback. Thanks! :)

johnhaddon · 2019-05-15T09:05:00Z

src/Gaffer/MetadataAlgo.cpp

@@ -261,7 +281,12 @@ Node *getNumericBookmark( ScriptNode *scriptNode, int bookmark )

 int numericBookmark( const Node *node )
 {
-	// Return the first one we find. There should only ever be just one matching value.
+	if( node->ancestor<Reference>() )


Likewise, then we could drop this check too.

johnhaddon · 2019-05-15T09:16:07Z

src/GafferBindings/MetadataBinding.cpp

+			result += boost::str(
+				boost::format( "Gaffer.MetadataAlgo.setNumericBookmark( %s.scriptNode(), %s, %s )\n" ) %
+				identifier %
+				stringValue %


stringValue is always "True" here. We need to serialise the correct integer for the bookmark instead. This code only works by fluke, and even then only for bookmark 1.

johnhaddon · 2019-05-15T09:17:17Z

python/GafferTest/MetadataAlgoTest.py

+		s = Gaffer.ScriptNode()
+		s["n1"] = Gaffer.Node()
+
+		Gaffer.MetadataAlgo.setNumericBookmark( s, 1, s["n1"] )


Let's also test a bookmark other than 1 in here, so we have some coverage for the bug noted above.

johnhaddon · 2019-05-17T08:28:00Z

Thanks Matti!

johnhaddon reviewed May 14, 2019

View reviewed changes

mattigruener force-pushed the numericBookmarksPersistence branch 3 times, most recently from badb70e to 63e57ec Compare May 14, 2019 18:54

johnhaddon requested changes May 15, 2019

View reviewed changes

themissingcow added this to In Progress in Work in Progress old May 15, 2019

themissingcow assigned themissingcow and mattigruener and unassigned themissingcow May 15, 2019

mattigruener force-pushed the numericBookmarksPersistence branch from 63e57ec to e41240b Compare May 15, 2019 19:00

MetadataAlgo : make numeric bookmarks persistent

0e65606

mattigruener force-pushed the numericBookmarksPersistence branch from e41240b to 0e65606 Compare May 16, 2019 18:41

johnhaddon merged commit f23b87f into GafferHQ:master May 17, 2019

Work in Progress old automation moved this from In Progress to Pending release May 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Numeric Bookmarks - Persistence #3157

Numeric Bookmarks - Persistence #3157

mattigruener commented May 11, 2019

mattigruener commented May 13, 2019

johnhaddon left a comment

johnhaddon May 14, 2019

johnhaddon May 14, 2019

johnhaddon May 14, 2019

mattigruener commented May 14, 2019

johnhaddon left a comment

johnhaddon May 15, 2019

johnhaddon May 15, 2019

mattigruener May 15, 2019

johnhaddon May 16, 2019

mattigruener May 16, 2019

johnhaddon May 15, 2019

mattigruener May 15, 2019

johnhaddon May 15, 2019

johnhaddon May 15, 2019

johnhaddon May 15, 2019

johnhaddon commented May 17, 2019

Numeric Bookmarks - Persistence #3157

Numeric Bookmarks - Persistence #3157

Conversation

mattigruener commented May 11, 2019

mattigruener commented May 13, 2019

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattigruener commented May 14, 2019

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnhaddon commented May 17, 2019