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.

Sign up for GitHub

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

Jump to bottom

FIX sdf/msdf shaders, add Shader.glslVersion #5328

Merged
merged 6 commits into from
Jul 18, 2023
Merged

FIX sdf/msdf shaders, add Shader.glslVersion #5328

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69b12ed
FIX sdf/msdf shaders, add Shader.glslVersion
nightgryphon Jul 6, 2023
f682e85
Update according to discussion
nightgryphon Jul 17, 2023
b0eecd8
Remove Shader.glslVersion
nightgryphon Jul 17, 2023
7384488
Cleanup. Huh...
nightgryphon Jul 17, 2023
db6d945
Formatting checks...
nightgryphon Jul 17, 2023
341d35b
Remove warning
nightgryphon Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/core/shader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ Shader.prototype = {
'gl_FragColor = vec4(1.0, 0.0, 1.0, 1.0);' +
'}',

glslVersion: null,
Copy link
Member

@dmarcos dmarcos Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want this to be configurable? Probably passing the correct value here should be enough:

https://github.com/aframevr/aframe/pull/5328/files#diff-28ca6f544f883c58911d21469d0462e0f7ff274b99f726eb276f7de5143f3ff2R62

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on if we want to support WebGL1 or not. (Though I'm not sure if A-Frame works with a WebGL1 context at the moment as I've never tried it)

Copy link
Member

Choose a reason for hiding this comment

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

This should not be part of the public API. Should check if the WebGLRenderer is webgl1 or not. Still I think is moot because as you said A-Frame doesn't offer a WebGL1 option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This definitely depends of decision to support WebGL1. If we drop WebGL1 support than sdf/msdf code should also be stripped/cleaned to remove GL1 shaders (which are differs from GL2 ones) and some startup checks for WebGL version has to be added to warn user of unsupported WebGL version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This THREE functionality looks a bit not clean as it is THREE who injects extra code in to shader so it is THREE job to inject code right way instead of forcing users to fill in the additional version parameter

Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be part of the public API. Should check if the WebGLRenderer is webgl1 or not.

It's a bit more subtle than that. The WebGLRenderer supports both WebGL 1 and WebGL 2 and will automatically pick one. The WebGL1Renderer class only exists to force WebGL 1. In other words, just checking .isWebGL1Renderer isn't sufficient, as you can have a normal WebGLRenderer with a WebGL 1 context (in case your browser/device doesn't support WebGL 2).

What we're discussing however is the glslVersion. This is tied to the WebGL version in the sense that you can only use ES 3.0 shaders with WebGL 2, but you can use ES 1.0 shaders on both WebGL 1 and 2. There are differences in syntax so you can't simply tag on a #version 300 es on an ES 1.0 shader.

So if we hide this and default to THREE.GLSL3 we're basically deciding two things at the same time:

  • A-Frame will only supports WebGL2 (which it might already depend in other areas of the code?)
  • A-Frame will only supports ES 3.0 shaders

Which I don't view as a problem, but is worth having clear in case someone wants to incorporate a (raw) ES 1.0 shader. And as @nightgryphon points out, if we do opt for this approach we should cleanup the sdf/msdf code and add an explicit WebGL2 check.

Copy link
Contributor

Choose a reason for hiding this comment

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

This THREE functionality looks a bit not clean as it is THREE who injects extra code in to shader so it is THREE job to inject code right way instead of forcing users to fill in the additional version parameter

Totally agree with you on this. I don't mind THREE.js injecting things into shaders, but it's surprising that they now also do this for RawShaderMaterial...


/**
* Init handler. Similar to attachedCallback.
* Called during shader initialization and is only run once.
Expand All @@ -56,7 +58,8 @@ Shader.prototype = {
// attributes: this.attributes,
uniforms: this.uniforms,
vertexShader: this.vertexShader,
fragmentShader: this.fragmentShader
fragmentShader: this.fragmentShader,
glslVersion: this.glslVersion
Copy link
Member

Choose a reason for hiding this comment

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

probably passing here THREE.GLSL3 is all the changes we need if I'm not missing anything. thoughts?

Copy link
Contributor Author

@nightgryphon nightgryphon Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hardcoding version instead of making default value for parameter can be not the best solution. What if somewhere in future we will get GLSL4,5,6 or sort of custom extensions which require to pass different version string? For me the right way was to respect original user shader code but THREE did this...

Also hardcoding version require cleanup for sdf/msdf shader modules to remove GL1 shaders and add GL version checks to their init constructors

Copy link
Contributor

Choose a reason for hiding this comment

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

Hardcoding in this instance is not too bad. There's almost certainly never going to be a newer version for WebGL with WebGPU on the horizon.

It all boils down to two separate decisions (see other comment thread): do we want to support ES 1.0 GLSL shaders with A-Frame and do we want to support WebGL1 (if that even works right now?).

Copy link
Member

Choose a reason for hiding this comment

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

We can hardcode for now or can also determine from the renderer (WebGL1, WebGL2, WebGPU) instead of a public API. sounds good? thanks for the patience

Copy link
Contributor

Choose a reason for hiding this comment

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

Did some testing and it seems A-Frame can (still) work fine with WebGL1, so I'd rather not break core components and try to postpone that till Three.js drops support (r163). Also found out that hardcoding it, actually breaks non-raw custom shaders as the GLSL 3.0 shader conversion that Three.js does ends up behaving slightly different :-/

Automatically determining it from the renderer won't be an option as the shader is written in a specific shading language. So either we expose a way for users to tell which language/version they used, or we restrict what A-Frame supports.

Basically the following support matrix exists:

WebGL 1.0 WebGL 2.0 WebGPU
ShaderMaterial (GLSL 1.0 /w 3.0 conversion) ✅/❌ (2) ✅ ❌
GLSL 1.0 ✅ ✅ ❌
GLSL 3.0 ❌ ⚠️ (1) ❌
NodeMaterial ? ✅ ✅
  1. Before the Three.js update the shader code itself could indicate the GLSL version in its source. Now that three.js injects other things at the start, it must also handle injecting the version directive. By extension this means that we must tell three.js which version it is.
  2. Depends on which GLSL features and keywords are used. The GLSL 3.0 conversion in Three.js only handles a small subset.

tl;dr I'd suggest the following:

Copy link
Member

Choose a reason for hiding this comment

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

let's go for the simple solution. thanks

For now set glslVersion: raw ? THREE.GLSL3 : undefined, fixing the problem for the majority of the users.
As a separate/follow-up: change the sdf and msdf shaders to non-raw shaders. This would remove the need to keep two almost identical copies around (GLSL1 and GLSL3 versions), restores WebGL1 compatibility, makes it easier to fix fog interaction (#4261), logarithmic depth (#4279) and color management.

});
return this.material;
},
Expand Down
7 changes: 4 additions & 3 deletions src/shaders/msdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var registerShader = require('../core/shader').registerShader;
var THREE = require('../lib/three');

var isWebGL2AVailable = !!document.createElement('canvas').getContext('webgl2');

Expand All @@ -15,7 +16,6 @@ var VERTEX_SHADER_WEBGL1 = [
].join('\n');

var VERTEX_SHADER_WEBGL2 = [
'#version 300 es',
'in vec2 uv;',
'in vec3 position;',
'uniform mat4 projectionMatrix;',
Expand Down Expand Up @@ -74,7 +74,6 @@ var FRAGMENT_SHADER_WEBGL1 = [
].join('\n');

var FRAGMENT_SHADER_WEBGL2 = [
'#version 300 es',
'precision highp float;',
'uniform bool negate;',
'uniform float alphaTest;',
Expand Down Expand Up @@ -135,5 +134,7 @@ module.exports.Shader = registerShader('msdf', {

vertexShader: VERTEX_SHADER,

fragmentShader: FRAGMENT_SHADER
fragmentShader: FRAGMENT_SHADER,

glslVersion: isWebGL2AVailable ? THREE.GLSL3 : null
});
7 changes: 4 additions & 3 deletions src/shaders/sdf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var registerShader = require('../core/shader').registerShader;
var THREE = require('../lib/three');

var isWebGL2AVailable = !!document.createElement('canvas').getContext('webgl2');

Expand All @@ -15,7 +16,6 @@ var VERTEX_SHADER_WEBGL1 = [
].join('\n');

var VERTEX_SHADER_WEBGL2 = [
'#version 300 es',
'in vec2 uv;',
'in vec3 position;',
'uniform mat4 projectionMatrix;',
Expand Down Expand Up @@ -114,7 +114,6 @@ var FRAGMENT_SHADER_WEBGL1 = [
].join('\n');

var FRAGMENT_SHADER_WEBGL2 = [
'#version 300 es',
'precision highp float;',
'uniform float alphaTest;',
'uniform float opacity;',
Expand Down Expand Up @@ -213,5 +212,7 @@ module.exports.Shader = registerShader('sdf', {

vertexShader: VERTEX_SHADER,

fragmentShader: FRAGMENT_SHADER
fragmentShader: FRAGMENT_SHADER,

glslVersion: isWebGL2AVailable ? THREE.GLSL3 : null
});