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

Move pen point difference calculation from shader code to JS #667

Merged

Conversation

adroitwhiz
Copy link
Contributor

Resolves

Resolves #642

Proposed Changes

This changes the pen line code so that the difference between pen points is calculated in JS instead of in the shader code.

I also stopped passing u_penPoints into the fragment shader (it hasn't needed that since last time I fixed this; I just forgot to remove it then), and clarified some of the comments.

Reason for Changes

If the pen points' coordinates are both somewhat large, calculating the difference between them will be inaccurate if done using the lower floating-point precision on GPUs. This messes up vertex shader calculations further down the line.

I'm pleasantly surprised by how simple the fix was here--it shouldn't decrease performance at all.

Test Coverage

Tested manually on an iPad Mini 5 (A2133) that was previously exhibiting the issue.

I'm giving up on coding and becoming a farmer. I hate this code so much
@adroitwhiz
Copy link
Contributor Author

After some investigation, it seems like the root cause of this is...

sigh...

Affected devices think that if you subtract a number from itself, you don't get 0.
This seems to be a shader miscompilation on Apple's side. This PR works around it. I'll try to contact Apple about this bug.

Copy link
Contributor

@fsih fsih left a comment

Choose a reason for hiding this comment

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

Nice! Looks reasonable to me!

Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

LGTM!

@adroitwhiz adroitwhiz merged commit ffddd7f into scratchfoundation:develop Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some pen strokes result in large, blurred circles on iPhone Safari
3 participants