GafferOSL : expand indexed primitive variables before shading. #2655

donboie · 2018-07-12T17:11:19Z

Bit of a bodge to fix a crash bug when processing indexed primvars in OSLObject.

johnhaddon

Thanks Don. I've noted a few potential issues that I think might be addressed by switching prepareShadingPoints to use ShadingEngine::needsAttribute() directly instead of requiredPrimvars. That avoids the linear search, doesn't require changes to hash/affects, and doesn't have any ambiguity around required vs resampled. Is there something I'm missing that would stop that from working?

johnhaddon · 2018-07-16T11:00:17Z

src/GafferOSL/OSLObject.cpp

-		// cast is ok - we're only using it to be able to reference the data from the shadingPoints,
-		// but nothing will modify the data itself.
-		shadingPoints->writable()[it->first] = boost::const_pointer_cast<Data>( it->second.data );
+		bool primvarUsed = std::find( primvarNames.begin(), primvarNames.end(), it->first ) != primvarNames.end();


Any quadratic complexity concerns here? Do we expect the number of primvars to always be low enough to get away with linear search?

johnhaddon · 2018-07-16T11:03:26Z

src/GafferOSL/OSLObject.cpp

@@ -254,7 +275,7 @@ IECore::ConstObjectPtr OSLObject::computeProcessedObject( const ScenePath &path,
 	PrimitiveVariable::Interpolation interpolation = static_cast<PrimitiveVariable::Interpolation>( interpolationPlug()->getValue() );

 	IECoreScene::ConstPrimitivePtr resampledObject = IECore::runTimeCast<const IECoreScene::Primitive>( resampledInPlug()->objectPlug()->getValue() );
-	CompoundDataPtr shadingPoints = prepareShadingPoints( resampledObject.get() );
+	CompoundDataPtr shadingPoints = prepareShadingPoints( resampledObject.get(), resampledNamesPlug()->getValue() );


If a plug is pulled on in compute(), it should also be hashed in hash(). I suspect we can get away with this here because it is hashed into resampledInPlug(), but it's best to stick to the rules - it's easy to get things subtly wrong otherwise. The same goes for affects().

johnhaddon · 2018-07-16T11:06:08Z

src/GafferOSL/OSLObject.cpp

+		}
+		else
+		{
+			// cast is ok - we're only using it to be able to reference the data from the shadingPoints,


We're adding the primvar here even if primvarUsed is false. That seems wrong, but if I understand correctly, is necessary because Constant primvars aren't listed in requiredPrimvars. In other words, requiredPrimvars is a misleading name - it's actually resampledPrimVars.

Can we not also omit the non-indexed primvars if needsAttribute() is false?

I though so also but I got a few test failures which I couldn't explain. Seems like we depend on all the privmars being present in the shadedPoint compound some how. I'll look into it further.

Thanks - I feel like there's either a hole in my understanding here, or a bug somewhere, so it'd be good to get to the bottom of it...

Looks like we need to have the "P" primvar to define the number of points to shade. Also I found problems with a unit test which read 'u' so have forced globals to be required to be on the safe side.

donboie · 2018-07-16T16:40:26Z

Thanks Don. I've noted a few potential issues that I think might be addressed by switching prepareShadingPoints to use ShadingEngine::needsAttribute() directly instead of requiredPrimvars. That avoids the linear search, doesn't require changes to hash/affects, and doesn't have any ambiguity around required vs resampled. Is there something I'm missing that would stop that from working?

Much better. I don't know why I was so quick to discount this approach - updates squashed into
eb7ffe3

johnhaddon · 2018-07-20T07:38:00Z

src/GafferOSL/ShadingEngine.cpp

@@ -1297,5 +1297,11 @@ bool ShadingEngine::needsAttribute( const std::string &scope, const std::string
 		return true;
 	}

+	// globals which are always needed
+	if ( name == "P" || name == "uv" || name == "u" || name == "v" || name == "N" )


I really want to limit the amount of unnecessary resampling and de-indexing we do - it's a major overhead. Can't we use the globals_needed query we're already using in ShadingEngine::queryContextVariablesAndAttributesNeeded() to make this more accurate?

Is something like 3815e9c better?

Yes. But I don't think the attribute/global distinction is relevant in terms of the ShadingEngine interface, because everything is passed in via that one container of points to be shaded. As a user of the class there is no distinction at all. I know needsAttribute() takes both scope and name, but actually that's misleading because scope is unused from the point of view of the client of the ShadingEngine. Likewise, the combined "uv" vs separate "u" and "v" consideration could be totally encapsulated within the ShadingEngine, and so could the requirement for "P". OSLObject just literally just need to call ShadingEngine::needsAttribute( primVarName ) and nothing else.

As a separate thing, I think it'd be good to remove the requirement for "P" by passing in a point count on its own, as it could reduce overhead in a lot of OSLImage cases. But that can wait. Let's just get it working so that we have a single ShadingEngine::needsAttribute( name ) call that encapsulates everything, hiding any distinction between globals and attributes...

Yes. But I don't think the attribute/global distinction is relevant in terms of the ShadingEngine interface, because everything is passed in via that one container of points to be shaded. As a user of the class there is no distinction at all. I know needsAttribute() takes both scope and name, but actually that's misleading because scope is unused from the point of view of the client of the ShadingEngine. Likewise, the combined "uv" vs separate "u" and "v" consideration could be totally encapsulated within the ShadingEngine, and so could the requirement for "P". OSLObject just literally just need to call ShadingEngine::needsAttribute( primVarName ) and nothing else.

right o

As a separate thing, I think it'd be good to remove the requirement for "P" by passing in a point count on its own,

Agreed

How about 78cff01 ?

Looks good - thanks Don!

- removed 'scope' parameter from needsAttribute as it always an empty string.

johnhaddon reviewed Jul 16, 2018

View reviewed changes

donboie force-pushed the OSLObjectIndexPrimVars branch from 53c9cd0 to eb7ffe3 Compare July 16, 2018 16:38

donboie force-pushed the OSLObjectIndexPrimVars branch 3 times, most recently from 4986baa to eb21320 Compare July 19, 2018 23:47

johnhaddon reviewed Jul 20, 2018

View reviewed changes

donboie force-pushed the OSLObjectIndexPrimVars branch from 3815e9c to 78cff01 Compare July 20, 2018 17:49

GafferOSL : expand indexed primitive variables before shading.

baa159e

- removed 'scope' parameter from needsAttribute as it always an empty string.

donboie force-pushed the OSLObjectIndexPrimVars branch from 78cff01 to baa159e Compare July 20, 2018 19:22

johnhaddon merged commit 8b25e6d into GafferHQ:master Jul 24, 2018

donboie mentioned this pull request Aug 22, 2018

0.47 Backport - GafferOSL : expand indexed primitive variables before shading. #2741

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GafferOSL : expand indexed primitive variables before shading. #2655

GafferOSL : expand indexed primitive variables before shading. #2655

donboie commented Jul 12, 2018

johnhaddon left a comment

johnhaddon Jul 16, 2018

johnhaddon Jul 16, 2018

johnhaddon Jul 16, 2018

johnhaddon Jul 17, 2018

donboie Jul 17, 2018

johnhaddon Jul 18, 2018

donboie Jul 19, 2018

donboie commented Jul 16, 2018 •

edited

Loading

johnhaddon Jul 20, 2018

donboie Jul 20, 2018

johnhaddon Jul 20, 2018

donboie Jul 20, 2018

donboie Jul 20, 2018

johnhaddon Jul 24, 2018

GafferOSL : expand indexed primitive variables before shading. #2655

GafferOSL : expand indexed primitive variables before shading. #2655

Conversation

donboie commented Jul 12, 2018

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

donboie commented Jul 16, 2018 • edited Loading

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

donboie commented Jul 16, 2018 •

edited

Loading