Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSLObject FaceVarying and Uniform interpolation #2113

Merged
merged 3 commits into from Jun 7, 2017

Conversation

@donboie
Copy link
Contributor

donboie commented Jun 6, 2017

This PR allows OSLObject to shade using Uniform and FaceVarying interpolations in addition to the preexisting Vertex mode.

I'm concerned with the performance regression when only reading already vertex interpolated variables. I have another branch were I query the variables read in the shader but I'm unsure at what point I should read these and set the names plug on the internal resample node. Hopefully performance hit is acceptable and we can address the performance in a subsequent PR?


"description",
"""
interpolation mode in which to process the object.

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Capitalise, remove blank lines.

Copy link
Member

johnhaddon left a comment

Thanks Don - this is going to be very handy.

I'm concerned with the performance regression when only reading already vertex interpolated variables.

Yeah, I think that's a very valid concern. My biggest worry would be significant numbers of constant primvars getting upsampled, leading to lots of extra work and big memory overheads. This is especially upsetting because we can already read constant primitive variables without needing to upsample.

I have another branch were I query the variables read in the shader but I'm unsure at what point I should read these and set the names plug on the internal resample node. Hopefully performance hit is acceptable and we can address the performance in a subsequent PR?

How about a compromise, where we only resample non-constant primitive variables? We can compute the list of non-constant primvars without needing access to any fancy information from the shader at all, and that will get the machinery in place for doing the fancier stuff later. The answer to "at what point I should set the names plug?" question is "in a compute for a private internal plug, which is connected to provide the input for the ResamplePrimitiveVariables.names plug". The GafferImage::Blur node is a decent example for this - it computes an internal __filterScale plug from the blur radius, and uses that to control an internal Resample node that does the actual hard work.

While we're talking about constant primvars, is there any reason not to provide that as an interpolation mode? Seems like it might be handy?

One last thing, I think it would have been worth squashing down to omit the dead-end commit (d884f8f) in the middle, since it's pretty much all gone by the end anyway. Presenting it that way would have made the unnecessary includes and the affects/hash omissions more obvious I think, and made the review a bit easier.

Cheers...
John

p.s. Apologies for the single comment prior to this review - I seem to have a habit of pressing the single comment button instead of the start a review button.

@@ -57,6 +57,7 @@ OSLObject::OSLObject( const std::string &name )
{
storeIndexOfNextChild( g_firstPlugIndex );
addChild( new ShaderPlug( "shader" ) );
addChild( new IntPlug( "interpolation", Plug::In, PrimitiveVariable::Invalid, PrimitiveVariable::FaceVarying ) );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

This is setting the default to Invalid and the Min to FaceVarying - seems like a bug?


CompoundDataPtr prepareShadingPoints( const Primitive *primitive, PrimitiveVariable::Interpolation interpolation )
{
//size_t numShadingPoints = primitive->variableSize( interpolation );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Dead code

@@ -38,12 +38,11 @@
#include "IECore/MeshPrimitive.h"
#include "IECore/CurvesPrimitive.h"
#include "IECore/PointsPrimitive.h"
#include "IECore/MeshAlgo.h"

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Presumably the Primitive headers above are no longer necessary either?

@@ -76,6 +76,9 @@ class OSLObject : public GafferScene::SceneElementProcessor
virtual void hashProcessedObject( const ScenePath &path, const Gaffer::Context *context, IECore::MurmurHash &h ) const;
virtual IECore::ConstObjectPtr computeProcessedObject( const ScenePath &path, const Gaffer::Context *context, IECore::ConstObjectPtr inputObject ) const;

GafferScene::ScenePlug *resampledInPlug();

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Should be private.

@@ -107,7 +80,19 @@ OSLObject::OSLObject( const std::string &name )
{
storeIndexOfNextChild( g_firstPlugIndex );
addChild( new ShaderPlug( "shader" ) );
addChild( new IntPlug( "interpolation", Plug::In, PrimitiveVariable::Invalid, PrimitiveVariable::FaceVarying ) );
addChild( new IntPlug( "interpolation", Plug::In, PrimitiveVariable::Vertex, PrimitiveVariable::Invalid, PrimitiveVariable::FaceVarying ) );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Ah, I see you fixed the earlier default/min/max problem here.

{
return inputObject;
}
const IECore::Primitive* resampledObject = IECore::runTimeCast<const IECore::Primitive>( resampledInPlug()->object( path ).get() );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

There are a few issues here :

  • We need to use ConstPrimitivePtr resampledObject so that we share ownership of the object for as long as we use it. This class of bug is really nasty, because often the central cache will keep the object alive and you won't notice during testing, until one day another thread causes a cache eviction and everything goes bang.
  • We should call resampledInPlug()->objectPlug()->getValue() rather than resampledInPlug()->object(). The latter is a convenience function which constructs a new temporary context to get an object from another location. Since we know we want the object from the current location, getValue() is sufficient, and cheaper.
  • The compute method now accesses resampledInPlug(), but this is not reflected in the affects() or hash() methods.
self.assertEqual( cubeObject["P_copy"].data[4], IECore.V3f( 0.0, 0.5, 0.0 ) + IECore.V3f( 1, 2, 3 ))
self.assertEqual( cubeObject["P_copy"].data[5], IECore.V3f( 0.0, -0.5, 0.0 ) + IECore.V3f( 1, 2, 3 ))

def testCanShadeFaceVaryingInterpolatedPrimitiveVariablesAsVertex( self ) :

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

Needs to be indented so it's a method and not a global function.

This comment has been minimized.

Copy link
@donboie

donboie Jun 6, 2017

Author Contributor

I should have squashed that intermediate step - sorry. I'm a little concerned sometimes I'm too eager to squash changes into fewer commits but in this case it would have made things clearer.

I'm unclear if I need to update affects & hash to consider the resampledInPlug?

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

I'm unclear if I need to update affects & hash to consider the resampledInPlug?

The best rule of thumb is to work backwards from the compute(), since that's the thing you're actually interested in implementing, and the rest is just an annoyance needed to keep caching and dirty propagation working. You only need to implement compute() for output plugs, and only then if they don't have an input connection. Since resampledInPlug() is an input plug and has an input connection, you definitely don't need to implement compute for it and therefore don't need to compute a hash() for it, or add it to the affected outputs in affects().

What I was trying to get it is that because you now access a value from resampledInPlug() in computeProcessedObject(), you also need to include it in the hash in hashProcessedObject() and ensure that it causes out.object to be dirtied in affects(). You're probably getting away with it for now because interpolationPlug() is approximating the same effect, but as we make the logic for which variables are resampled more complex, getting this right will become more important.

This comment has been minimized.

Copy link
@donboie

donboie Jun 6, 2017

Author Contributor

Thanks for the explanation

f["paths"].setValue( IECore.StringVectorData( [ '/cube' ] ) )
o["filter"].setInput( f["out"] )

# ensure the source position primitive variable interpolation is ver

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

is vertex?


inPoint = GafferOSL.OSLShader( "InPoint" )
s.addChild( inPoint )
inPoint.loadShader( "ObjectProcessing/InPoint", keepExistingValues = True )

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 6, 2017

Member

keepExistingValues is redundant here. Let's remove it so as not to confuse folks using the test cases as documentation/examples.

@donboie donboie force-pushed the donboie:OSLObjectInterpolation branch from f2827e3 to 7a1cf1f Jun 6, 2017
@donboie

This comment has been minimized.

Copy link
Contributor Author

donboie commented Jun 6, 2017

Hopefully I've addressed all of your comments apart from the Constant interpolation. I attempted to support this and noticed we'd either have convert the ScalarData primvars to VectorData in OSLObject for use by the shading engine or update the shading engine to treat scalars as single element vectors.

I am struggling a little to think of examples where we'd want to shade constant resampled primvars so how do you feel about deferring this until we have a clear need of it?

Copy link
Member

johnhaddon left a comment

Nearly there. GitHub has hidden my most important comment in a "Show outdated" section - it's the one about hashProcessedObject().

It looks like we're still missing the dependents declaration for the resampledInPlug()->objectPlug() in affects() as well. Recap :

  • If the compute for plug X gets a value from plug Y :
    • The hash for plug X must include the hash for plug Y
    • The affects() method must add plug X to the outputs when called with input plug Y

It looks like there are several plugs that declare an affect on outPlug()->objectPlug() in affects() already, but they're all grouped into their own if() clause, which is confusing. Ideally it would look like this :

if( input == inPlug()->shaderPlug() ||
    input == inPlug()->transformPlug() ||
    input == interpolationPlug() ||
    input == resampledInPlug()->objectPlug()
)
{
    outputs.push_back( outPlug()->objectPlug() );
}

I've been thinking that just as we have hashObject() and computeObject() that are delegated to by hash() and compute(), we should also have affectsObject() that is delegated to by affects(). Arranging the code like this will make the transition easier, and in the shorter term makes it easier to verify that affects/hash/compute match.

@@ -169,6 +225,7 @@ void OSLObject::hashProcessedObject( const ScenePath &path, const Gaffer::Contex

interpolationPlug()->hash( h );
h.append( inPlug()->fullTransformHash( path ) );
h.append( resampledInPlug()->fullAttributesHash( path ) );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 7, 2017

Member

I think we've got our wires crossed somewhere - in computeProcessedObject() we call resampledInPlug()->objectPlug()->getValue(), so here we need to call resampledInPlug()->objectPlug()->hash( h ), and not anything to do with the attributes.

Recap :

  • A ScenePlug has various child plugs each representing a property of a location
    • objectPlug() represents geometry/cameras etc
    • transformPlug() represents the local transform
    • boundPlug() represents the local bounding box
    • attributesPlug() represents the local attributes. Note that "attributes" here is used in the RenderMan sense of inheritable properties that are orthogonal to geometry/transforms etc. This is different to the "everything is an attribute" approach taken by certain other packages.
  • The fullAttributes/fullAttributesHash methods walk all parent locations, getting the local attributes (or their hash) and combining them with the result so far. They're rarely needed in a computation context.

if (!prim)
{
static_cast<StringPlug *>( output )->setValue( "" );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 7, 2017

Member

output->setToDefault() might be slightly better here, because it can reuse the internal default value rather than needing to construct a new one.

"description",
"""
Interpolation mode in which to process the object.
All primitive variables are resampled to match the selected interpolation.

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 7, 2017

Member

All non-constant primitive variables. Maybe we could explain a bit better what we mean by "in which to process the object" as well? How about something like this?

"""
The interpolation type of the primitive variables created by this node. For instance, Uniform interpolation means that the shader is run once per face on a mesh, allowing it to output primitive variables with a value per face. All non-constant input primitive variables are resampled to match the selected interpolation so that they can be accessed from the shader.
"""

@@ -151,6 +166,8 @@ void OSLObject::hashProcessedObject( const ScenePath &path, const Gaffer::Contex
{
shader->attributesHash( h );
}

interpolationPlug()->hash( h );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 7, 2017

Member

Thanks for squashing that middle commit down. One note for the future - getting affects/hash/compute to match is one of the most important things in coding a node, so it's best not to split the hash/compute changes across commits like this as it makes review hard. I'd have been quite happy to see this commit squashed into the next too...

@donboie donboie force-pushed the donboie:OSLObjectInterpolation branch from 7a1cf1f to bfa74c1 Jun 7, 2017
@donboie

This comment has been minimized.

Copy link
Contributor Author

donboie commented Jun 7, 2017

I really hope I understand now 🤦‍♂ .Thanks so much for detailed explanations and patience.

Are you happy with leaving the constant interpolation for the future?

Copy link
Member

johnhaddon left a comment

OK, nearly there! Just the one comment about the duplicated push_backs now. Chances are I'll be done for the day by the time you've fixed that and the tests go through, so if you could merge once they've passed that would be great.

Are you happy with leaving the constant interpolation for the future?

Yeah, for sure. When I suggested it I hadn't realised that it would require changes to the ShadingEngine...

Cheers...
John

}
else if( input == inPlug()->transformPlug() )
{
outputs.push_back( outPlug()->objectPlug() );

This comment has been minimized.

Copy link
@johnhaddon

johnhaddon Jun 7, 2017

Member

There seem to be three calls to outputs.push_back( outPlug()->objectPlug() ) here - we just need the one, as in my example from the previous review.

@donboie donboie force-pushed the donboie:OSLObjectInterpolation branch from bfa74c1 to 2fd2c43 Jun 7, 2017
@donboie donboie merged commit 42365f2 into GafferHQ:master Jun 7, 2017
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.