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

Patch 1 #8

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Patch 1 #8

wants to merge 9 commits into from

Conversation

bmschmidt
Copy link

See #7

@@ -1,7 +1,7 @@
float cubicInOut(float t) {
return t < 0.5
? 4.0 * t * t * t
: 0.5 * pow(2.0 * t - 2.0, 3.0) + 1.0;
: 1. - 4.0 * pow(1. - t, 3.0);
Copy link
Member

Choose a reason for hiding this comment

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

What would you think instead of

Suggested change
: 1. - 4.0 * pow(1. - t, 3.0);
: 1. - 4.0 * pow(abs(1. - t), 3.0);

or my favorite solution, I think,

float cubicInOut(float t) {
  float tm1 = t - 1.0;
  return t < 0.5
    ? 4.0 * t * t * t
    : 1.0 + 4.0 * tm1 * tm1 * tm1;
}

For the reasons mentioned in #7 (comment), namely that even though people shouldn't plug in t < 0 or t > 1, they certainly will, perhaps won't notice they've done it, and it's at least nice if the returned value isn't formally undefined and platform-dependent (which, in my experience usually gets caught years later when a developer gets a new computer, runs the same code, and suddenly things look all wrong).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for keeping this library up. That makes sense.

I like the tm1 formula with no pows at all better, too. But I like '1-t' instead of 't - 1' because the formulas can stay the same whether it's cubic or quartic, rather than flipping the sign in the final addition. Commits below do it with an explicit if block to avoid an unnecessary subtraction when t < .5.

@bmschmidt
Copy link
Author

This also fixes a problem where the visual tests didn't run because the frag shader was misnamed in the code.

Copy link
Member

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

Really nitpicking here, just for the fifth powers. Stack overflow says maybe pow(abs(x), 5.0) is appropriate so that you can let the glsl compiler do the optimization if that's a thing it feels is necessary, but I truly don't think it matters as long as the result is correct, so that I support this PR no matter what. Thanks for working on this!

float qinticIn(float t) {
return pow(t, 5.0);
float quinticIn(float t) {
return t*t*t*t*t;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return t*t*t*t*t;
float t2 = t * t;
return t2 * t2 * t;

return 16.0 * t * t * t * t * t;
}
float u = 1.0 - t;
return 1.0 - 16.0 * u * u * u * u * u;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return 1.0 - 16.0 * u * u * u * u * u;
float u2 = u * u;
return 1.0 - 16.0 * u2 * u2 * u;

: -0.5 * pow(2.0 * t - 2.0, 5.0) + 1.0;
float quinticInOut(float t) {
if (t < 0.5) {
return 16.0 * t * t * t * t * t;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return 16.0 * t * t * t * t * t;
float t2 = t * t;
return 16.0 * t2 * t2 * t;

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