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

[DYN-7923] CurveMapper node #15863

Merged
merged 23 commits into from
Mar 6, 2025

Conversation

ivaylo-matov
Copy link
Contributor

Purpose

This PR addresses DYN-7923. It implements the Graph Map node from Celery as a built-in Dynamo node.

The new CurveMapper node redistributes a series of numbers based on predefined curve equations. The available curve types include Linear, Bezier, Sine, Cosine, Parabolic, Perlin Noise, Gaussian, Power, and Square Root. Users can adjust the curves interactively using interactive handles. Additional controls allow users to lock curves, reset them, and resize the node.

The UI, styling, and tooltips align with the design specifications outlined in Figma.

As agreed, resources and unit tests will be included in subsequent PRs.

DYN-7923-CurveMapper

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

This PR addresses DYN-7923. It implements the Graph Map node from Celery as a built-in Dynamo node.

The new CurveMapper node redistributes a series of numbers based on predefined curve equations. The available curve types include Linear, Bezier, Sine, Cosine, Parabolic, Perlin Noise, Gaussian, Power, and Square Root. Users can adjust the curves interactively using interactive handles. Additional controls allow users to lock curves, reset them, and resize the node.

The UI, styling, and tooltips align with the design specifications outlined in Figma.

As agreed, resources and unit tests will be included in subsequent PRs.

Reviewers

@QilongTang
@reddyashish

FYIs

@dnenov
@achintyabhat

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7923

@QilongTang QilongTang added this to the 3.5 milestone Feb 27, 2025
if (ResetButton.ToolTip is ToolTip resetTooltip)
{
resetTooltip.Content = curveMapperNodeModel.IsLocked
? "The curve has been locked and cannot be reset. Please unlock the curve first."
Copy link
Contributor

@reddyashish reddyashish Feb 28, 2025

Choose a reason for hiding this comment

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

These need to be moved to resources, so that it can be localized.


namespace DSCore.CurveMapper
{
public class PerlinNoiseCurve : CurveBase
Copy link
Contributor

Choose a reason for hiding this comment

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

@ivaylo-matov Thank you for adding detailed comments, can we add a class summary to the different curve models used in this node. Would be useful as they are exposed as public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @reddyashish, I've added the summaries.
The resorces and localization will come in the next PR. Hope that's okay.

@reddyashish
Copy link
Contributor

Looks good overall, can merge and get it to testing once the comment is addressed.

@reddyashish
Copy link
Contributor

Some failing tests here:
Screenshot 2025-03-04 at 11 51 44 AM

Seems unrelated, but can you verify them locally?

@ivaylo-matov
Copy link
Contributor Author

@reddyashish , locally only one of those tests fails. The fail is not related to CurveMapper though, as it fails with or without it.

image

@reddyashish
Copy link
Contributor

Merging this PR to get the testing started on the new node.
The failing test is not related and will look at which PR caused that rergression.

@reddyashish reddyashish merged commit 859c383 into DynamoDS:master Mar 6, 2025
22 of 24 checks passed
zeusongit added a commit that referenced this pull request Mar 7, 2025
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.

3 participants