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

Fix pow(x,y) undefined when x<0 #3

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

Conversation

felthy
Copy link

@felthy felthy commented Nov 14, 2017

I used quartic-out in a project and when testing on some older hardware (ATI Radeon HD 2600 Pro 256MB, in a 2008 iMac), I found that it gave crazy results:

quartic-out

I narrowed it down to the pow() call, then found that actually pow(x,y) is undefined when x<0. This pull request just tweaks the shaders so they avoid passing x<0 to pow, which fixes all eases on this hardware:

all eases after 9523fc4

I also added a blue.glsl to get the tests working.

@rreusser
Copy link
Member

Only feedback would be the possibility that they could have explicit abs (or alternatively clamp(t, 0.0, 1.0)) so that they extend smoothly to negative numbers. Like if you have floating point -0.0000001, they'll still break badly on some mobile devices, right?

@felthy
Copy link
Author

felthy commented Nov 14, 2017

Hi, thanks for reviewing! I considered adding clamp but decided not to for a couple of reasons - I think it's a well known contract with easing functions that t must be in the range[0,1], so it might be better for performance to leave that validation up to the user. And the existing codebase didn't validate input so I didn't want to meddle.

@rreusser
Copy link
Member

Okay, a tentative conclusion on this one:

#8 addresses this by just getting pow() to work correctly, perhaps even just by unrolling powers. Not sure whether pow(abs(__), 3.0) or x * x * x is optimal, but that seems a distant second to just getting it consistent. The solution, therefore, is not to clamp but just to make sure the results are not undefined outside the range [0, 1].

@rreusser
Copy link
Member

Will close this once one patch or another is merged.

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