LocaliseAttributes / AttributeQuery / AttributeTweaks : Inherit global attributes #6502
Conversation
There was a problem hiding this comment.
Thanks Murray!
ShaderQuery & ShaderTweaks would also benefit from this new behaviour if we plan to add a global plug to ShaderAssignment
I think perhaps we should do ShaderQuery and ShaderTweaks now, since it's already possible to make a global shader attribute by other means? Doing it now also means less compatibility worries than if we were to do it after adding ShaderAssignment.global.
src/GafferScene/ScenePlug.cpp
Outdated
| @@ -452,6 +461,21 @@ IECore::CompoundObjectPtr ScenePlug::fullAttributes( const ScenePath &scenePath | |||
| path.pop_back(); | |||
| } | |||
|
|
|||
| if( withGlobalAttributes ) | |||
| { | |||
| for( const auto &g : globals()->members() ) | |||
There was a problem hiding this comment.
I think we may have a dangling pointer issue here. Until C++23, which fixes the issue, I believe this is equivalent to this :
auto &&r = globals()->members();
for( auto it = begin( r ); it != end( r ); ++it )
{
}
globals() returns an owning Ptr, and we need to keep that alive for as long as we access members().
There was a problem hiding this comment.
Fixed in 62e5fa1
There was a problem hiding this comment.
Sorry, we have crossed wires here. My code example wasn't the intended fix, rather it was demonstrating what the original code was equivalent to, and why that was a problem. To be more clear :
// This creates a dangling reference. `globals()` returns `ConstCompoundObjectPtr`,
// and we need to keep that around to maintain ownership of the result, for as long
// as we access it. But after this line has run, the Ptr is gone, and we're referencing
// something that may no longer be alive.
auto &&r = globals()->members();
// This is what we actually want to do.
ConstCompoundObjectPtr g = globals();
for( const auto &m : g->members() )
src/GafferScene/ScenePlug.cpp
Outdated
| @@ -532,6 +556,18 @@ IECore::MurmurHash ScenePlug::fullAttributesHash( const ScenePath &scenePath ) c | |||
| path.pop_back(); | |||
| } | |||
|
|
|||
| if( withGlobalAttributes ) | |||
| { | |||
| for( const auto &g : globals()->members() ) | |||
There was a problem hiding this comment.
As above, I think we have a lifetime issue here.
There was a problem hiding this comment.
As above, fixed in 62e5fa1
|
Oh, one other thing I forgot. All the Changes.md updates will need moving into a new section since we release 1.6.0.0a1 the other day. |
murraystevenson
force-pushed
the
inheritGlobalAttributes
branch
from
b0b4473 to
9f53f89
Compare
|
Thanks John! Rebased to bring up to date and fix the Changes.md entries, feedback addressed inline.
Added those in cb0dea3 and 366c924. It turned out that as ShaderQuery uses an internal AttributeQuery it just needed a test and a tweak to the menu. |
|
Thanks Murray - changes all look good except for the Ptr mixup I've commented inline. Could you squash everything down while fixing that so we're ready to merge please? Thanks! |
murraystevenson
force-pushed
the
inheritGlobalAttributes
branch
from
9f53f89 to
4f129a9
Compare
|
Apologies for the crossed wires! That should be properly addressed now and all is squashed down ready to merge. |
`resultMembers.insert()` won't clobber existing members, so we can remove the additional test.
johnhaddon
force-pushed
the
inheritGlobalAttributes
branch
from
4f129a9 to
2dc8851
Compare
|
Thanks for the update - rebased to fix the Changes.md conflict, and merged. |
This adds support for inheriting/localising global attributes in LocaliseAttributes, AttributeQuery and AttributeTweaks. After a bit of dliberation, I've made this behaviour optional for LocaliseAttributes via a new
includeGlobalAttributesplug that defaults off for backwards compatibility, but is userDefaulted on for newly created nodes. This additional complexity seems reasonable for LocaliseAttributes, as it's already quite a simple node, and with this plug, LocaliseAttributes could be used in concert with AttributeQuery/AttributeTweaks/SubTree/etc in more complex cases where you require a very specific flavour of localisation.This PR does introduce two breaking changes of behaviour, an AttributeQuery with
inheritenabled will now return a result when querying an attribute that doesn't exist in the scene hierarchy but does exist in the globals, and AttributeTweaks would now not create an attribute inCreateIfMissingmode iflocalisewas enabled and there was a global attribute of the same name. These may be edge-case enough that we don't need to go to great lengths to provide a compatibility mode, or overly complicate the nodes. I've reached out to a few users to gauge their opinions...Future work would be to include a
useFallbackplug on AttributeQuery to allow the query to return the registered default value as a fallback, and changing theexistsplug to asourceplug similar to what we've added to CameraQuery. ShaderQuery & ShaderTweaks would also benefit from this new behaviour if we plan to add aglobalplug to ShaderAssignment, though LocaliseAttributes would help with this in the meantime.