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

nightgryphon · 2023-07-06T09:30:16Z

Description:
Fix shaders according to THREE changes. Fixes Text component.

Changes proposed:

added Shader.glslVersion. Shaders should use this to specify version instead of shader #version string
fixed sdf/msdf shaders used in Text component

dmarcos · 2023-07-06T19:20:02Z

src/core/shader.js

@@ -45,6 +45,8 @@ Shader.prototype = {
      'gl_FragColor = vec4(1.0, 0.0, 1.0, 1.0);' +
    '}',

+  glslVersion: null,


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

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)

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

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.

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

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.

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...

dmarcos · 2023-07-08T00:14:55Z

src/core/shader.js

@@ -56,7 +58,8 @@ Shader.prototype = {
      // attributes: this.attributes,
      uniforms: this.uniforms,
      vertexShader: this.vertexShader,
-      fragmentShader: this.fragmentShader
+      fragmentShader: this.fragmentShader,
+      glslVersion: this.glslVersion


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

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

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?).

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

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 ? ✅ ✅

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.

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:

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 (Scene fog not working on a-text #4261), logarithmic depth (Text not rendering when logarithmicDepthBuffer is enabled #4279) and color management.

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.

dmarcos · 2023-07-13T20:32:53Z

@nightgryphon can we incorporate these changes:

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 (Scene fog not working on a-text #4261), logarithmic depth (Text not rendering when logarithmicDepthBuffer is enabled #4279) and color management.

notice that undefined a better choice than null that in JS means the absence of an object type {}

nightgryphon · 2023-07-15T11:27:06Z

@dmarcos this will make all raw shaders treated as GLSL3. Didn't this cause compatibility issues for anyone who uses raw shaders?
For thouse with GLSL3 shaders they will get duplicate version string which will be clearly visible in error log so not so bad.
For GLSL1... I'm not sure if GLSL1 shaders will compile with GLSL3 version string but if so than for GLSL1 shaders this will produce the unexpected shaders behavior without any error message which is awful

Regarding sdf/msdf shaders this will not fix them and text component. The minimum is to remove version string from GLSL3 shader code. But it is better also to drop GLSL1 shaders just to be sure using right shader code in any case to prevent hard to detect bugs.

As an alternative we can just extract version from shader fixing shader code at the same time. Should work for most cases. Updating sdf/msdf is not required and can be postponed.

    // THREE r154 require '#version' as standalone value in glslVersion
    var version = undefined;
    for (var name of ['vertexShader', 'fragmentShader'] ) {
        var Lines = this[name].split("\n");
        var pos = Lines[0].indexOf('#version');

        if (pos >= 0) {
            version = Lines.shift().substr(pos+9);
            this[name] = Lines.join("\n");
        }
    }

here is the commit i can make PR from a6d735c

mrxz · 2023-07-15T12:15:04Z

@nightgryphon The idea is indeed both glslVersion: raw ? THREE.GLSL3 : undefined and removing the #version 300 es lines from the sdf/msdf shaders. Apologies for not making that clear.

This does indeed mean that third party raw shaders will require changes, though I think you'll find that not many people use raw shaders in the first place. For GLSL3 shaders they will simply have to remove the #version 300 es line (which is a change people using THREE.js will need to make anyway). For GLSL1 shaders, they'll either have to rewrite them or override init() and construct their own RawShaderMaterial.

I'd rather not introduce workarounds or special behaviour into A-Frame for the purpose of retaining old Three.js behaviour. While it's annoying that many of the breaking changes in A-Frame come from (breaking) changes in Three.js, going against it would result in workarounds and fixes piling up over time. And given that A-Frame offers virtually no abstraction over the Three.js shaders I don't think it makes sense to maintain this old behaviour indefinitely.

nightgryphon · 2023-07-16T08:18:40Z

Hm. Okay. At least that fixes the current problem with text.
So this comes to be like this
master...nightgryphon:aframe-local:FIX_Shaders3
Tested this with my project. Seems to work.
Should i create PR ?

PS Just notice regarding not to create THREE abstraction layer. On the one hand we pass through the untouched shaders code but on the other hand we abstract users from the glslVersion property. Looks a bit confusing but Ok. It works.

dmarcos · 2023-07-16T16:11:50Z

@nightgryphon can you modify this PR?

nightgryphon · 2023-07-17T16:55:36Z

A bit messy but done. Huh...

dmarcos · 2023-07-17T19:03:44Z

src/core/shader.js

@@ -50,11 +50,16 @@ Shader.prototype = {
   * Called during shader initialization and is only run once.
   */
  init: function (data) {
+    if (this.raw) {


Not sure this message is very valuable. Perhaps better making a note in the docs. I'm also happy to rip the band-aid and perhaps starting to say that WebGL1 is deprecated and WebGL2 is the way to go.

This definitely has to be added to docs. But i think keeping such warning just for a few versions can save some headache to those who still using old shaders and has to migrate.

Not sure warning is super descriptive for the avg. dev. It is extra code to maintain and eventually have to remember to remove when obsolete.

What we usually do is to add deprecations in the release notes for those migrating https://github.com/aframevr/aframe/releases That's evergreen info.

The warning isn't particular useful as it will always be printed. It might even confuse people seeing the warning when everything is correct. The fact that a big error is printed by Three.js down the line is probably enough. Anyone writing their own raw shaders will either understand the error or look it up and find the relevant Three.js and A-Frame issues. Those using older shaders from others will most likely open an issue about that error message anyway.

Documenting this change and mentioning it in the release notes is key, as that will help anyone updating and can easily be linked/pointed to when people encounter this.

nightgryphon · 2023-07-18T08:15:22Z

Ok. I've removed the warning

@mrxz the idea of that warning was regarding migrating from GLSL1 shaders where THREE most likely will not print any error but shader will be compiled with GLSL3 header line instead.
For the raw shader user the situation will look like for some misterious reason his custom shader starts to behave wrong. If user is at debugging stage the AFrame changes will be the last thing user will think of. Like it was for me with "VR rotating world" thing. On the other hand that's can indeed be confusing when such warning provided for native AFrame materials.

mrxz · 2023-07-18T09:19:18Z

the idea of that warning was regarding migrating from GLSL1 shaders where THREE most likely will not print any error but shader will be compiled with GLSL3 header line instead.

GLSL1 shaders can't be compiled as GLSL3 shaders even with the #version line in place as many of the keywords are completely different (e.g. attribute -> in, varying -> in/out, etc...). So in that case there will still be an error message in the console, albeit a different one.

(Note that with non-raw ShaderMaterial, Three.js provides conversions for GLSL1 keywords. That's why you can still use and see keywords like attribute, varying, etc in shaders, but that's not the case for RawShaderMaterial)

nightgryphon · 2023-07-18T13:26:12Z

@mrxz Oh. That's makes things clear. Thanks!

dmarcos · 2023-07-18T19:40:12Z

Thanks so much for the patience and being so accomodating!

FIX sdf/msdf shaders, add Shader.glslVersion

69b12ed

dmarcos reviewed Jul 6, 2023

View reviewed changes

dmarcos reviewed Jul 8, 2023

View reviewed changes

nightgryphon added 4 commits July 17, 2023 16:33

Update according to discussion

f682e85

Remove Shader.glslVersion

b0eecd8

Cleanup. Huh...

7384488

Formatting checks...

db6d945

This was referenced Jul 17, 2023

RawShaderMaterial in Three.js 0.154.1 appears to have a regression #5339

Closed

Text component broken at master (src/shaders/sdf.js msdf.js) #5325

Closed

dmarcos reviewed Jul 17, 2023

View reviewed changes

Remove warning

341d35b

dmarcos merged commit c185a2b into aframevr:master Jul 18, 2023

This was referenced Nov 17, 2023

aframe 1.5.0 THREE.Material: parameter 'glslVersion' has value of undefined. supermedium/aframe-environment-component#96

Closed

Use correct default value (null) for glslVersion for non-raw shaders #5388

Merged

mrxz mentioned this pull request Jan 31, 2024

Text not rendering in VR mode #5260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nightgryphon commented Jul 6, 2023

dmarcos Jul 6, 2023 •

edited

Loading

mrxz Jul 7, 2023

dmarcos Jul 8, 2023

nightgryphon Jul 8, 2023

nightgryphon Jul 8, 2023

mrxz Jul 8, 2023

mrxz Jul 8, 2023

dmarcos Jul 8, 2023

nightgryphon Jul 8, 2023 •

edited

Loading

mrxz Jul 8, 2023

dmarcos Jul 10, 2023

mrxz Jul 10, 2023

dmarcos Jul 11, 2023

dmarcos commented Jul 13, 2023 •

edited

Loading

nightgryphon commented Jul 15, 2023 •

edited

Loading

mrxz commented Jul 15, 2023

nightgryphon commented Jul 16, 2023 •

edited

Loading

dmarcos commented Jul 16, 2023

nightgryphon commented Jul 17, 2023

dmarcos Jul 17, 2023

nightgryphon Jul 17, 2023

dmarcos Jul 17, 2023

mrxz Jul 18, 2023

nightgryphon commented Jul 18, 2023 •

edited

Loading

mrxz commented Jul 18, 2023

nightgryphon commented Jul 18, 2023

dmarcos commented Jul 18, 2023

	WebGL 1.0	WebGL 2.0	WebGPU
ShaderMaterial (GLSL 1.0 /w 3.0 conversion)	✅/❌ (2)	✅	❌
GLSL 1.0	✅	✅	❌
GLSL 3.0	❌	⚠️ (1)	❌
NodeMaterial	?	✅	✅

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

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

Conversation

nightgryphon commented Jul 6, 2023

dmarcos Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightgryphon Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tl;dr I'd suggest the following:

Choose a reason for hiding this comment

dmarcos commented Jul 13, 2023 • edited Loading

nightgryphon commented Jul 15, 2023 • edited Loading

mrxz commented Jul 15, 2023

nightgryphon commented Jul 16, 2023 • edited Loading

dmarcos commented Jul 16, 2023

nightgryphon commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightgryphon commented Jul 18, 2023 • edited Loading

mrxz commented Jul 18, 2023

nightgryphon commented Jul 18, 2023

dmarcos commented Jul 18, 2023

dmarcos Jul 6, 2023 •

edited

Loading

nightgryphon Jul 8, 2023 •

edited

Loading

dmarcos commented Jul 13, 2023 •

edited

Loading

nightgryphon commented Jul 15, 2023 •

edited

Loading

nightgryphon commented Jul 16, 2023 •

edited

Loading

nightgryphon commented Jul 18, 2023 •

edited

Loading