Replace Spline With Ramp For Shader Parameters #1499
Conversation
The actual classes were removed long ago in 10.0.0-a12
danieldresser-ie
force-pushed
the
splinesRound2
branch
from
4fb3a18 to
c198a76
Compare
|
Huh, the build failure on Mac OS is pretty weird ... it's in a file I shouldn't have affected at all, and don't immediately see any way I've touched ... it's in CurvesPrimitiveEvaluator ( the other sort of spline ), and it appears to be a naming clash with something named "linear", but the only thing I'm aware of having added named linear is IECore::RampInterpolation::Linear ( with a capital, in a namespace, and not even included here? ). Odd. |
johnhaddon
left a comment
johnhaddon
left a comment
There was a problem hiding this comment.
Thanks Daniel! A few comments inline - I think the main topic of discussion is how doable it would be to move some of the Ramp stuff to ShaderNetworkAlgo...perhaps we can talk about that in the meeting that started one minute ago :)
When converting a ramp to OSL conventions, is the variable holding it still named "ramp", or is it called a "spline" once it's converted?
I'd be inclined to call the OSL variables spline*, to indicate the transformation.
There's an outstanding TODO in src/IECoreGL/ShaderStateComponent.cpp, which currently converts SplineData to textures...but if it's only being used elsewhere, then maybe anyone who is still using it would benefit more from keeping the old behaviour?
That would be my guess - I think the most likely client would also be using SplineParameter. Maybe folks at IE will know more, but I don't know of any Gaffer-side uses.
Huh, the build failure on Mac OS is pretty weird .
Yeah, this is nothing to do with the PR - it was broken previously. My bad - I merged a PR from Paul without noticing the failure on Mac, and need to find time to fix it (since we aren't shipping anything on Mac, it's not urgent).
| case RampffDataTypeId : | ||
| return | ||
| typename Detail::DespatchTypedData< Functor, RampffData, ErrorHandler > | ||
| ::template Func<Enabler>()( static_cast<RampffData *>( data ), functor, errorHandler ); | ||
| case RampfColor3fDataTypeId : | ||
| return | ||
| typename Detail::DespatchTypedData< Functor, RampfColor3fData, ErrorHandler > | ||
| ::template Func<Enabler>()( static_cast<RampfColor3fData *>( data ), functor, errorHandler ); | ||
| case RampfColor4fDataTypeId : | ||
| return | ||
| typename Detail::DespatchTypedData< Functor, RampfColor4fData, ErrorHandler > | ||
| ::template Func<Enabler>()( static_cast<RampfColor4fData *>( data ), functor, errorHandler ); |
There was a problem hiding this comment.
I'd be open to omitting this - despatchTypedData() is deprecated so it might even be considered counter-productive to support new types in it.
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Vertical space is at a premium on most monitors, and three blank lines doesn't really convey anything more than one blank line would.
| class IECORE_EXPORT Ramp | ||
| { | ||
|
|
||
| public : |
There was a problem hiding this comment.
Oh, right, I'd copied this from a Gaffer struct, and I guess I forgot to change the identation. I don't really remember why I switched it to a class, but I'm guessing it had private members at some point during the refactor - maybe it makes sense to keep it as a class in case it gets private members again at some point in the future? I'm also not quite sure why our indentation is different for structs, but that's a topic for another time, I'll just make this indentation consistent for now.
There was a problem hiding this comment.
I don't think our indentation is different for structs, at least not intentionally. For another time for sure, but I think maybe what I would like the most is to change the way we do things like so :
struct MyThing // Everything is a struct
{
MyThing(); // So now public stuff doesn't need to be doubly indented.
private :
m_stuff; // Only private stuff needs double indentation.
}
| /// A Ramp represents a spline-like curve as it is represented in a simple UI: with a set of independent | ||
| /// control points, and an interpolation type selected from RampInterpolation. |
There was a problem hiding this comment.
I think it's worth mentioning that another motivation is removing the need to store duplicate endpoints to ensure interpolation to the final values.
| // In order to write shader parameters to renderers or USD, we need to convert Ramps to a convention | ||
| // that can be stored in raw OSL. Based on the arguments to the OSL spline() function, we've come up | ||
| // with a convention for storing a string basis and separate position and value arrays, where the | ||
| // the position and value arrays contain any duplicated end points necessary for the resulting OSL | ||
| // spline to cover the full range of the ramp. |
There was a problem hiding this comment.
"We've come up with a convention" seems a bit misleading - the only convention here is the one defined by OSL's spline() itself. We're literally just generating parameters that will reproduce the ramp exactly in a call to spline().
| // \todo - I guess the only way to handle this properly would be to do the end point duplication | ||
| // and conversion from monotone to bezier in our OSL shaders ( like the PRMan and 3delight shader | ||
| // libraries do ). |
There was a problem hiding this comment.
Do PRMan and 3delight really do monotone->bezier per sample, or are you just talking about endpoint duplication for that part? If they don't, this just seems like another argument against having ever had monotone interpolation...
| // TODO - this probably should actually be updated to take Ramp, but in order to test | ||
| // that, I need to figure out where this is used. |
There was a problem hiding this comment.
I took a quick look through Gaffer, and I don't think it's used there. It was definitely in use at IE at some point - maybe for IELight visualisers?
|
|
||
| if( const ShaderNetwork *shaderNetwork = runTimeCast<const ShaderNetwork>( result.get() ) ) | ||
| { | ||
| bool needsDeprecatedSplineConversion = false; |
There was a problem hiding this comment.
Why do we do this needsDeprecatedSplineConversion() dance? Couldn't convertDeprecatedSplines() just do nothing if nothing is needed?
|
|
||
| if( needsDeprecatedSplineConversion ) | ||
| { | ||
| ShaderNetworkPtr resultNetwork = shaderNetwork->copy(); |
There was a problem hiding this comment.
It looks like we started with a non-const ObjectPtr that we own uniquely (via Object::load()), so do we really need to take a copy?
| ShaderNetwork *network, const TypedData<TypedSpline> *splineData, | ||
| const IECore::CompoundDataMap &newParameters, const IECore::InternedString &splineParameterName, | ||
| template< typename TypedRamp > | ||
| std::tuple< InternedString, size_t, size_t, size_t > createRampInputAdapter( |
|
One more quick note - this PR needs to target |
2 participants
We shouldn't merge this before we fully sort out the Gaffer side as well, to make sure that everything works with the new API ( I think I've got everything working in Gaffer now, but it's not cleaned up enough to put the PR up yet ). But the Cortex side should be good to review now.
Known questions currently: