Skip to content

Commit

Permalink
Document violations of ArrayPlug API
Browse files Browse the repository at this point in the history
And outline a possible path forwards. In practice, we're currently getting away with the violations because the serialisations for these nodes all call `addQuery()` to resize the arrays upfront, so that the arrays already have the required size before the `resize()` call is executed.
  • Loading branch information
johnhaddon committed Aug 27, 2024
1 parent 1ffae29 commit 9cbde14
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 2 deletions.
4 changes: 4 additions & 0 deletions src/Gaffer/ArrayPlug.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ bool ArrayPlug::acceptsChild( const GraphComponent *potentialChild ) const
return true;
}

/// \todo We could beef up these checks to check any descendants of
/// `potentialChild` and to check the default value etc. We can't do that
/// until we fix a few violators of our constraint that all children are
/// identical - see `ShaderQuery::ShaderQuery`.
return potentialChild->typeId() == m_elementPrototype->typeId();
}

Expand Down
1 change: 1 addition & 0 deletions src/Gaffer/ContextQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ ContextQuery::ContextQuery( const std::string &name ) : Gaffer::ComputeNode( nam
{
storeIndexOfNextChild( g_firstPlugIndex );

/// \todo See notes in `ShaderQuery::ShaderQuery`.
addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );
addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );
}
Expand Down
2 changes: 1 addition & 1 deletion src/GafferBindings/Serialisation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ namespace
// and more readable, and opens the possibility of omitting the
// overhead of the names entirely one day.
/// \todo Consider an official way for GraphComponents to opt in
/// to this behaviour.
/// to this behaviour. Perhaps this could just be driven by metadata?
bool keyedByIndex( const GraphComponent *parent )
{
return
Expand Down
1 change: 1 addition & 0 deletions src/GafferScene/OptionQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ OptionQuery::OptionQuery( const std::string &name ) : Gaffer::ComputeNode( name
storeIndexOfNextChild( g_firstPlugIndex );

addChild( new ScenePlug( "scene" ) );
/// \todo See notes in `ShaderQuery::ShaderQuery`.
addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );

addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );
Expand Down
1 change: 1 addition & 0 deletions src/GafferScene/PrimitiveVariableQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ PrimitiveVariableQuery::PrimitiveVariableQuery( const std::string& name )
storeIndexOfNextChild( g_firstPlugIndex );
addChild( new ScenePlug( "scene" ) );
addChild( new Gaffer::StringPlug( "location" ) );
/// \todo See notes in `ShaderQuery::ShaderQuery`.
addChild( new Gaffer::ArrayPlug( "queries", Gaffer::Plug::Direction::In,
nullptr, 1, std::numeric_limits< size_t >::max(), Gaffer::Plug::Flags::Default, false ) );
addChild( new Gaffer::ArrayPlug( "out", Gaffer::Plug::Direction::Out,
Expand Down
10 changes: 9 additions & 1 deletion src/GafferScene/ShaderQuery.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,16 @@ ShaderQuery::ShaderQuery( const std::string &name ) : Gaffer::ComputeNode( name
addChild( new StringPlug( "location" ) );
addChild( new StringPlug( "shader" ) );
addChild( new BoolPlug( "inherit", Plug::In, false ) );
/// \todo This is violating the ArrayPlug requirements by not providing an `elementPrototype`
/// at construction. And we later violate it further by creating non-homogeneous elements in
/// `addQuery()`. We're only using an ArrayPlug because we want the serialisation to use numeric
/// indexing for the children of `queries` and `out` - since the serialisation uses `addQuery()`
/// to recreate the children, and that doesn't maintain names. So perhaps we can just use a
/// ValuePlug instead of an ArrayPlug, and add a separate mechanism for requesting that children use
/// numeric indices separately (see `keyedByIndex()` in `Serialisation.cpp`).
///
/// The same applies to OptionQuery, PrimitiveVariableQuery and ContextQuery.
addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );

addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits<size_t>::max(), Plug::Flags::Default, false ) );

AttributeQueryPtr attributeQuery = new AttributeQuery( "__attributeQuery" );
Expand Down

0 comments on commit 9cbde14

Please sign in to comment.