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

Add shader precision #60

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

Add shader precision #60

wants to merge 2 commits into from

Conversation

dmnsgn
Copy link
Member

@dmnsgn dmnsgn commented Jan 13, 2019

  • Set default precision to 'highp' as a context option (possibility to override it on context creation)
  • Set ctx.capabilities.maxPrecision
  • Expose getMaxPrecision function with overridable precision parameter:
    • function also have 'highp' as default and will fallback to mediump or lowp
    • precision parameter allows override even if a higher precision is available

Ground work for: pex-gl/pex-renderer#122

@vorg
Copy link
Member

vorg commented Jan 14, 2019

Couple of notes:

getMaxPrecision is one of a kind API (getSomething), what about exposing capabilities.maxPrecision and letting the user handle that for now? What would be better pex-context way?

It seems that the getMaxPrecision(precision) function is coming from ThreeJS where they issue a warning if the max precision doesn't match the requested material precision.

Are we sure our aim is to support such devices with no highp? Can we do some more research when using mediump is beneficial?

@@ -269,6 +270,28 @@ function createContext (opts) {
capabilities.maxColorAttachments = gl.getParameter('MAX_COLOR_ATTACHMENTS')
}

function getMaxPrecision(precision = 'highp') {
Copy link
Member

Choose a reason for hiding this comment

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

getMaxPrecision is one of a kind API. More in the PR comments

Copy link
Member

@vorg vorg left a comment

Choose a reason for hiding this comment

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

I agree we need to handle it somehow just not sure about API yet.

@dmnsgn
Copy link
Member Author

dmnsgn commented Jan 14, 2019

I exposed the getMaxPrecision as the user needs it when overriding a material with a certain precision. See:

I agree it feels slightly weird because it is one of a kind but I couldn't find a better way to make it accessible to pex-renderer without having to duplicate the gl precision check code. Only other acceptable way I see so far would be to make the function a separate package.

Are we sure our aim is to support such devices with no highp? Can we do some more research when using mediump is beneficial?

We encountered some mobile devices with only mediump available on cx.

It seems that the getMaxPrecision(precision) function is coming from ThreeJS where they issue a warning if the max precision doesn't match the requested material precision.

We can add a warning as well or use a debug feature somehow.

@vorg
Copy link
Member

vorg commented Jan 18, 2019

I feel like we are trying to make things complicated here. In somehow controversial discussions like this one it's useful to think i case of scenarios.

From my understanding some older devices might not support highp. I've never heard of a device not supporting mediump. I would imagine the getShaderPrecisionFormat for all precisions is there only for completeness.

I would suggest leaving only capabilities.maxPrecision renamed tocapabilities.maxFragmentPrecision. One issue with that is we introduce string values e.g. "highp". Those while same as glsl code should probably be enums (pex-context way). I assume strings are use for pex-renderer convenience.

There is actually built-in glsl constant GL_FRAGMENT_PRECISION_HIGH as explained in "How to write portable WebGL". So you can query highp availability in the shader:

#if GL_FRAGMENT_PRECISION_HIGH == 1
    // highp is supported
#else
    // high is not supported
#endif

That means we could potentially have matching

capabilities.fragmentPrecisionHigh = true/false

* 'master' of github.com:pex-gl/pex-context: (25 commits)
  Update README.md
  Update README.md
  Remove glslify and clean up examples
  Fix security vulnerabilities for examples
  Update examples deps
  Documentation: add ctx.capabilities
  Documentation: unify parameter column names: property, info, type, default?
  Add Encoding enum to docs + Format markdown enums
  Clean up
  Fix linting
  Fix log namespace for framebuffer
  Remove standard
  Use arrowParens always
  Add lint and format convenience scripts
  Add eslint standard config
  Format examples
  Add prettier and eslint
  Add assert to FBO status check
  Remove temp debug context code
  Update picking example to new renderbuffer API
  ...

# Conflicts:
#	index.js
@dmnsgn
Copy link
Member Author

dmnsgn commented Mar 15, 2019

From my understanding some older devices might not support highp. I've never heard of a device not supporting mediump. I would imagine the getShaderPrecisionFormat for all precisions is there only for completeness.

We can get rid of 'lowp'.


I would suggest leaving only capabilities.maxPrecision renamed tocapabilities.maxFragmentPrecision. One issue with that is we introduce string values e.g. "highp". Those while same as glsl code should probably be enums (pex-context way). I assume strings are use for pex-renderer convenience.

getMaxPrecision actually checks for both vertex and fragment precision (gl.getShaderPrecisionFormat(gl.VERTEX_SHADER, gl.HIGH_FLOAT).precision > 0 && gl.getShaderPrecisionFormat(gl.FRAGMENT_SHADER, gl.HIGH_FLOAT)) so I would keep maxPrecision. I am not aware of any case where it would be different from vert to frag but then why does the API exists in this way?


There is actually built-in glsl constant GL_FRAGMENT_PRECISION_HIGH as explained in "How to write portable WebGL". So you can query highp availability in the shader:

#if GL_FRAGMENT_PRECISION_HIGH == 1
    // highp is supported
#else
    // high is not supported
#endif

So that kind of make useless to have it in pex-context? We could just add a direct replacement if highp is not supported in pex-renderer shaders:

#ifndef GL_FRAGMENT_PRECISION_HIGH
#define highp mediump
#endif

varying highp vec3 vPositionWorld;
// would become
varying mediump vec3 vPositionWorld;

That means we could potentially have matching

capabilities.fragmentPrecisionHigh = true/false

You mean no ctx. capabilities.maxPrecision but just ctx. capabilities.fragmentPrecisionHigh instead?

@vorg
Copy link
Member

vorg commented Mar 20, 2019

  • inspect filament shaders to see what they are using

https://google.github.io/filament/webgl/suzanne.html

No checks in GLSL but it doesn't mean they don't do them in JS. As mentioned in docs they default to mediump but specify highp for position related data attributes

#version 300 es
precision mediump float;
precision mediump int;
layout(std140) uniform FrameUniforms
{
highp mat4 viewFromWorldMatrix;
highp mat4 worldFromViewMatrix;
highp mat4 clipFromViewMatrix;
highp mat4 viewFromClipMatrix;
highp mat4 clipFromWorldMatrix;
highp mat4 worldFromClipMatrix;
highp mat4 lightFromWorldMatrix;
highp vec4 resolution;
highp vec3 cameraPosition;
highp float time;

@vorg
Copy link
Member

vorg commented Mar 20, 2019

Yes, only fragmentPrecisionHigh=true/false. Eg. BabylonJS has highPrecisionShaderSupported. This avoids confusion with maxFragmentPrecision as gl.getShaderPrecisionFormat is precision is not just high/med/low but high with 32 bits or high with 24 bits etc..

We would then use GL_FRAGMENT_PRECISION_HIGH to fallback to mediump when we write cross platform code in pex-renderer.

This flag would be used more for low end device sniffing than overwriting shader precisions as doing it automatically and globally most likely will not work and just cause artifacts to show up.

@dmnsgn dmnsgn added type/feat A new feature and removed feature labels Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants