Skip to content

Interpolator : Remove overzealous asserts#1527

Open
johnhaddon wants to merge 1 commit intoImageEngine:mainfrom
johnhaddon:removeInterpolationAsserts
Open

Interpolator : Remove overzealous asserts#1527
johnhaddon wants to merge 1 commit intoImageEngine:mainfrom
johnhaddon:removeInterpolationAsserts

Conversation

@johnhaddon
Copy link
Copy Markdown
Member

It's perfectly reasonable to want to interpolate "off the end", and in fact we need to in CurvesAlgo::convertPinnedToNonPeriodic(). I intended to take care of this in #1526 but got carried away and merged before I had done so.

It's perfectly reasonable to want to interpolate "off the end", and in fact we need to in `CurvesAlgo::convertPinnedToNonPeriodic()`. I intended to take care of this in ImageEngine#1526 but got carried away and merged before I had done so.
@johnhaddon johnhaddon self-assigned this Mar 27, 2026
@danieldresser-ie
Copy link
Copy Markdown
Contributor

I wasn't really thinking about needing this when I reviewed #1526, but if that change is the only place where it's needed to relax this constraint, I'm not really sure if this is really an improvement. Do we have anywhere where we actually want to interpolate off the end of a non-linear curve? That's probably what this assert was originally intended to prevent?

In #1526, interpolator( B, A, 2, C ) is less succinct than just saying C = A - 2 * B, and I'm not sure it's really much clearer.

I also don't see any real problem with merging this, just figured I'd mention that it doesn't feel like an obvious improvement to me.

@johnhaddon
Copy link
Copy Markdown
Member Author

I'm not sure if you're objecting to the change to CubicInterpolator, or to both. I don't need the CubicInterpolator change, so could drop that. But I can't just say C = A - 2 * B because that's not defined for types like Box, Matrix and Quat, which is what the various Interpolator specialisations were taking care of for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants