Skip to content

Bookmarks : Ignore bookmarks from read-only boxes in general#4320

Merged
johnhaddon merged 3 commits intoGafferHQ:0.60_maintenancefrom
ivanimanishi:bookmarkFixes
Aug 23, 2021
Merged

Bookmarks : Ignore bookmarks from read-only boxes in general#4320
johnhaddon merged 3 commits intoGafferHQ:0.60_maintenancefrom
ivanimanishi:bookmarkFixes

Conversation

@ivanimanishi
Copy link
Member

This PR changes the bookmark setting code so that bookmarks are ignored not only if they are inside a Reference, but also if they are inside a read-only box (like the DynamicBoxes used at IE).

@ivanimanishi ivanimanishi force-pushed the bookmarkFixes branch 2 times, most recently from d1f22c5 to dceeb6d Compare August 19, 2021 23:54
@johnhaddon
Copy link
Member

Thanks Ivan! There was a conflict in the Changes.md file, so I've pushed a fix for that, and also taken the liberty of moving a couple of things into anonymous namespaces in the .cpp files (so we don't export the symbols unnecessarily). Will merge when CI completes...

@johnhaddon
Copy link
Member

Error: FAIL: testLifetimes (GafferUITest.NodeUITest.NodeUITest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/__w/gaffer/gaffer/build/python/GafferUITest/NodeUITest.py", line 47, in testLifetimes
    self.assertNodeUIsHaveExpectedLifetime( Gaffer )
  File "/__w/gaffer/gaffer/build/python/GafferUITest/TestCase.py", line 128, in assertNodeUIsHaveExpectedLifetime
    nodeUI = GafferUI.NodeUI.create( script["node"] )
  File "/__w/gaffer/gaffer/build/python/GafferUI/NodeUI.py", line 152, in create
    assert( 0 )
AssertionError

Well, this is a mess I hadn't anticipated. It turns out that making a static metadata registration can cause havoc with IECore::RunTimeTyped's internal registry that services queries like RunTimeTyped::baseTypeIds(). It's a variation of the static initialisation order fiasco. This is the sequence of events :

  • Reference.cpp registers the Reference node type with RunTimeTyped, via GAFFER_NODE_DEFINE_TYPE( Reference );. This turns into a call to RunTimeTyped::registerType() under the hood.
  • Then Reference.cpp registers some metadata against ReferenceTypeId. The Metadata system wants to signal this change, and as part of that wants to query the base types of that TypeId, so it calls RunTimeTyped::baseTypeIds().
  • RunTimeTyped::baseTypeIds() works by populating itself behind the scenes the first time it is called, and then reusing that result forevermore. So at this point we are baking the list of base types for Reference.
  • Alas, there's no guarantee that Node or SubGraph are registered with RunTimeTyped at this point, because they are registered in a different translation unit.
  • Hence RunTimeTyped bakes an incomplete list of bases, and simple queries fail forever more. In this case, it claims that Reference doesn't derive from Node, which means we can't find a NodeUI for it.

I'm not sure where to go from here. It's definitely possible to improve this situation in RunTimeTyped, because we know that if we don't reach RunTimeTyped when compiling baseTypeIds, that it's incomplete. But you might prefer not to wait for a Cortex patch, in which case the simplest short term fix is probably to move the metadata registration into Gaffer/__init__.py.

This is more generic than specifically checking for References.
Otherwise the flag is not properly set if we don't load GafferUI.
If the file is exported with childNodesAreReadOnly metadata set to True,
we need to reset it to the proper value when loading it as a Reference.
@ivanimanishi
Copy link
Member Author

Updated with the Gaffer.__init__.py approach for the metadata registration.

@johnhaddon johnhaddon merged commit d339e6f into GafferHQ:0.60_maintenance Aug 23, 2021
@ivanimanishi ivanimanishi deleted the bookmarkFixes branch August 23, 2021 16:10
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.

2 participants