Skip to content

CurvesPrimitive clients : Support WrapMode enum#6855

Merged
johnhaddon merged 12 commits into
mainfrom
curvePinning
Mar 27, 2026
Merged

CurvesPrimitive clients : Support WrapMode enum#6855
johnhaddon merged 12 commits into
mainfrom
curvePinning

Conversation

@johnhaddon

Copy link
Copy Markdown
Member

Companion PR to ImageEngine/cortex#1526. This adds support for rendering pinned curves, and a new CurvesInterpolation node for changing the basis and wrap of existing curves.

@danieldresser-ie danieldresser-ie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

"""
When converting Pinned curves to NonPeriodic, adds the "phantom" vertices
so that the curves continue to interpolate to their original endpoints.
""",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing this makes me wonder is whether it would make sense to have an expand option that affects the conversion from periodic to non-periodic ( either broadening the scope of this plug, or adding a second one ). I guess the practical answer is that while adding phantom vertices to periodic curve that has been converted to non-periodic is a mathematically very similar operation, it's far less likely that a user would have need of it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I do think this would be a nice to have. I think the ideal might be to generalise it completely to a single preserveShape boolean, where we also adjust vertex positions to account for any change to the basis matrix. I had a request in with our resident maths expert to see what that would entail, but haven't heard back yet 😝.

I think all that can wait though - as you say, periodic curves don't seem to come up much.

Treating OptionalValuePlug as a terminal is consistent with the way we treat NameValuePlug. With that done, promoting the documentation from the `value` plug fixes the test failure, but also makes the tooltip available more easily in the UI, without having to turn the `enabled` switch on.
@johnhaddon johnhaddon merged commit afbf31d into main Mar 27, 2026
5 of 6 checks passed
@github-project-automation github-project-automation Bot moved this from Pending Review to Pending release in Work in Progress Mar 27, 2026
@murraystevenson murraystevenson deleted the curvePinning branch March 28, 2026 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Pending release

Development

Successfully merging this pull request may close these issues.

2 participants