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

Fixed curvePoint and curveTangent ignoring curveTightness #5638

Merged
merged 4 commits into from Dec 14, 2022

Conversation

sparshg
Copy link
Contributor

@sparshg sparshg commented Mar 16, 2022

Resolves #5601

Changes:

curveTightness was being ignored in curvePoint and in curveTangent. Fixed by using catmull-rom basis matrix

Screenshots of the change:

Before

Screenshot 2022-03-17 at 12 59 37 AM

After

Screenshot 2022-03-17 at 12 59 41 AM

Code for example

function draw() {
 curveTightness(-1);
 curve(5, 26, 73, 24, 73, 61, 15, 65);

 x = curvePoint(5, 73, 73, 15, 0.4);
 y = curvePoint(26, 24, 61, 65, 0.4);
 tx = curveTangent(5, 73, 73, 15, 0.4);
 ty = curveTangent(26, 24, 61, 65, 0.4);

 a = atan2(ty, tx);
 line(-cos(a) * 20 + x, -sin(a) * 20 +y, cos(a) * 20 + x, sin(a) * 20 + y);
 circle(x, y, 5); 
}

PR Checklist

@welcome
Copy link

welcome bot commented Mar 16, 2022

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@sparshg sparshg changed the title Fixed curvePoint and curveTangent ignoringcurveTightness Fixed curvePoint and curveTangent ignoring curveTightness Mar 16, 2022
@sparshg sparshg changed the title Fixed curvePoint and curveTangent ignoring curveTightness Fixed curvePoint and curveTangent ignoring curveTightness Mar 16, 2022
Copy link

@owenmcateer owenmcateer left a comment

Choose a reason for hiding this comment

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

Tested as working!
Great fix, I just ran into this bug myself.

@Qianqianye
Copy link
Contributor

Thanks @sparshg. I am inviting the Core stewards to review this PR @limzykenneth, @davepagurek, @jeffawang.

@davepagurek davepagurek self-requested a review December 13, 2022 17:38
Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

Just double checked the math, looks good! Thanks for taking this on!

In case anyone in the future is looking back on this PR wanting to double check these values, a typical Catmull-Rom matrix looks like this:

$$\begin{bmatrix} 0 & 1 & 0 & 0 \\ -\tau & 0 & \tau & 0 \\ 2\tau & \tau - 3 & 3 - 2 \tau & -\tau \\ -\tau & 2 - \tau & \tau - 2 & \tau \end{bmatrix}$$

And p5's curveTightness (called $s$ in this PR) translates to the tension $\tau$ with this formula:

$$s = -2\tau + 1$$

(The idea, mentioned in the curveTightness docs, is to make $s = 0$ correspond to the common default $\tau = 0.5$, and $s = 1$ gives straight lines through the curve vertices.)

@davepagurek davepagurek merged commit 3748490 into processing:main Dec 14, 2022
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.

curvePoint does not adjust to curveTightness
4 participants