CurvesPrimitive : Add WrapMode enum

[johnhaddon]

This extends the pre-existing periodic/nonperiodic boolean to support the additional concept of a Pinned wrap mode as defined by UsdGeomBasisCurves. This defines implicit "phantom vertices" to ensure that the rendered/evaluated curve interpolates all the way to its endpoints without having to manually double up points. It is so much nicer to work with, and I have no idea why we haven't all been doing this all along.

There are knock-on effects to a bunch of curve-consuming code, and there I've focussed most on those I know to be relevant to Gaffer. I'm sure performance could be improved in places, but I hope this is in a reasonable place to merge as a first pass, so I can get onto some more pressing priorities.

I'll open a companion PR for Gaffer shortly, so you'll be able to test this all in practice.

[johnhaddon]

Debug build is failing an assertion :

test (CurvesAlgoTest.CurvesAlgoConvertPinnedToNonPeriodicTest.test) ... python: include/IECore/Interpolator.inl:44: void IECore::LinearInterpolator<T>::operator()(const T&, const T&, double, T&) const [with T = Imath_3_1::Vec3<float>]: Assertion `x <= 1.0' failed.

Seems it might be reasonable to just remove the assertion?

[danieldresser-ie]

All looks good to me!

[danieldresser-ie]

Am I correct in thinking that your modified "phantom" basis matrices assign a weight of 0 to the standin verts, and we just need to stick something in here so we have the right number of segments? So rather than using the index of the closest point for the index of the standin vert, we would get the same result if we just used vert 0 for every standin vert?

ie:

vertIds.push_back( 0 );		
vertIds.push_back( baseIndex + pi );
vertIds.push_back( baseIndex + pi + 1 );
vertIds.push_back( numSegments > 1 ? baseIndex + pi + 2 : 0 );

If I'm correct in thinking this value is unused, that might be a bit clearer?

[johnhaddon]

You're right that the vert is weighted to 0, but I don't think "I know the weight is going to be zero so I'm putting nonsense here" is an improvement in clarity. I quite liked that repeating the vertex implied pinning, even if the mechanism is more subtle in reality. I concede that that can be argued both ways though.

I'm no expert in GPUs, but I would assume it's better to read contiguous memory than it is to jump back to the zeroth index repeatedly. Unless I'm mistaken, I think that is a decent tie-breaker.

[danieldresser-ie]

It might be worth being more specific about the pros and cons in this comment?

To my understanding:
Cons: the resulting curve shape will be subtly different for an indexed curve ( perhaps this is implied by saying we're not generating interpolated values, but I'm not sure I would immediately have understood that if not for the prior conversation ).
Pros: simplicity, preserving of vertex sharing

[johnhaddon]

I've pushed f1e703e with those elaborations.

[danieldresser-ie]

I do think using consistent nonsense as a marker of values that are unused is an improvement in clarity, but your argument for performance is persuasive. Probably should be mentioned in a comment though, since it's certainly not obvious that those values are unused.

Otherwise, LGTM.

[johnhaddon]

Probably should be mentioned in a comment though, since it's certainly not obvious that those values are unused.

I've updated the comment to mark the unused values and explain why they are unused. Merging so I can update the Gaffer PR with a suitable dependencies package.