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

optimize utils.droots #218

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

optimize utils.droots #218

wants to merge 1 commit into from

Conversation

F-star
Copy link

@F-star F-star commented Aug 4, 2024

No description provided.

Copy link
Owner

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

Please update your description to explain why you filed this and what you think the new code does that the old code doesn't.

PRs are absolutely welcome, but I need to know why you're suggesting these changes, because looking at what you changed this is almost certainly motivated by fixing a bug related to taking the square root of a negative number, so you'll have to include (or at the very least talk about) a test case where the current code fails, and this new code doesn't.

@F-star
Copy link
Author

F-star commented Aug 5, 2024

Hello, this is a small optimization.

I found that utils.droots() is executed when bezier.extrema() is called.

Sometimes, bezier.extrema() returns [NaN, NaN], which is then discarded in the filter method, like this:

result[dim] = result[dim].filter(function(t2) {
  return t2 >= 0 && t2 <= 1;
});

So I think we can optimize it.

When the square of the "delta of the quadratic formula" is negative, it means that the quadratic equation has no real roots, and sqrt will return NaN when the input is a negative number.

It's a good idea to return [] in advance.

utils.droots() is only executed by bezier.extrema(), so this change will not break any existing functionality.

@F-star F-star requested a review from Pomax August 5, 2024 02:42
@Pomax
Copy link
Owner

Pomax commented Aug 6, 2024

Cheers. Do you have an example curve/code snippet we can add to the tests for this?

@F-star
Copy link
Author

F-star commented Aug 6, 2024

Well, this is not a bug. bezier.extrema() returns the correct result every time.

I just made a small change to utils.droots() to ensure it does not return an array with NaN values.

new Bezier(
  [
    {
      x: 100,
      y: 206,
    },
    {
      x: 121,
      y: 47,
    },
    {
      x: 141,
      y: 273,
    },
    {
      x: 215,
      y: 348,
    },
  ]
).extrema()

The example above will get an array with NaN in the X dimension in the droots method, but bezier.extrema() will filter it. You can console.log before filter to find this case.

image

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