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

default to highp values in vertex shaders #2117

Merged
merged 7 commits into from
Feb 25, 2016
Merged

default to highp values in vertex shaders #2117

merged 7 commits into from
Feb 25, 2016

Conversation

lucaswoj
Copy link
Contributor

No description provided.

@ansis
Copy link
Contributor

ansis commented Feb 12, 2016

If a uniform is used in both the vertex and fragment shader it needs to have the same precision in both. The precision for these uniforms should be set to mediump in the vertex shader. I think this could be causing the errors.

I think highp is guaranteed for vertex shaders. We should check the spec and drop the replace(/ highp /g, ' mediump ').

@ansis ansis changed the title Always use highp values in vertex shaders when supported [DO NOT MERGE] default to highp values in vertex shaders Feb 23, 2016
@ansis
Copy link
Contributor

ansis commented Feb 23, 2016

afd74e7 fixes the shader compilation errors

Since vertex shaders always have highp we don't need. 5f7b700 See 4.5.2 in https://www.khronos.org/files/opengles_shading_language.pdf

399ab75 lowers precision where I think it's safe

@ansis
Copy link
Contributor

ansis commented Feb 23, 2016

I think this should be ready for merging. @lucaswoj want to review my changes?

@kkaefer

  • does switching to highp as the default vertex precision sound good?
  • do these precision qualifiers look good? 399ab75

@lucaswoj
Copy link
Contributor Author

LGTM.

🚢 once @kkaefer has a chance to weigh in

@kkaefer
Copy link
Member

kkaefer commented Feb 25, 2016

I don't have any objections, but I also don't know what all of the implications are; if we see any rendering artifacts, we'd probably have to look at the qualifiers on a case-by-case basis. I looked through 399ab75 and all of the qualifiers you added make sense.

@lucaswoj
Copy link
Contributor Author

Did a quick sanity check on my iPhone. This branch looks 👍

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.

3 participants