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

Enables WebGL 2 by default #10894

Merged
merged 42 commits into from
Jan 13, 2023
Merged

Enables WebGL 2 by default #10894

merged 42 commits into from
Jan 13, 2023

Conversation

sanjeetsuhag
Copy link
Contributor

@sanjeetsuhag sanjeetsuhag commented Nov 8, 2022

Fixes #10887, #10064

Overview

This PR enables WebGL 2 by default. The main changes here are the upgrade of shaders to GLSL 300 from GLSL 100. Instead of using modernizeShader to upgrade the GLSL version at runtime, from now, when a WebGL 1 context is detected, shaders will be downgraded using demodernizeShader.

To-Do

  • Upgrade GLSL shader files
  • Upgrade JS shader code
  • Implement demodernize shaders
  • Fix tests
  • Test Sandcastles
  • Update documentation/PR description

Testing

To test all Sandcastles with a WebGL 1 context, use this git patch and use this command:

git apply --ignore-space-change --ignore-whitespace sandcastle_webgl1.patch

Known Issues

@cesium-concierge
Copy link

Thanks for the pull request @sanjeetsuhag!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@sanjeetsuhag sanjeetsuhag marked this pull request as ready for review November 18, 2022 01:31
@sanjeetsuhag
Copy link
Contributor Author

sanjeetsuhag commented Dec 9, 2022

@lilleyse The code changes are done. I'm going through the documentation and fixing a couple of the known issue listed above. I think this would be a good time for you take a pass at this PR. I've retained the modernizeShader file only for reference during review.

@sanjeetsuhag
Copy link
Contributor Author

@lilleyse I've updated the PR and added a benchmark. It's ready for another look.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 5, 2023

I was spot checking some sandcastles and noticed that Elevation Band Material doesn't work anymore. This might be an isolated case because it involves the material system, but could you do a final pass over all the sandcastles just in case?

Otherwise I just had one comment about GL_EXT_frag_depth handling and this should be good to merge soon.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 10, 2023

Along with the Elevation Band Material problem, @ggetz and I noticed 2 more issues after going through the rest of the Sandcastles.

1. Labels appear more aliased.

This appears in several sandcastles. Here is a screenshot from the "Labels" Sandcastle, in main:
Main -Labels - Cesium Sandcastle

The same sandcastle in webgl2-default:
WebGL2-default -Labels - Cesium Sandcastle

2. The "Cesium Inspector" sandcastle is broken

Clicking on "Show Frustums" gives the following error:

RuntimeError: Fragment shader failed to compile.  Compile log: ERROR: 0:5: 'out_FragColor' : undeclared identifier
ERROR: 0:5: 'rgb' :  field selection requires structure, vector, or interface block on left hand side
ERROR: 0:5: 'assign' : l-value required (can't modify a const)
ERROR: 0:5: 'assign' : cannot convert from 'uniform highp 3-component vector of float' to 'const highp float'
ERROR: 0:6: 'out_FragColor' : undeclared identifier
ERROR: 0:6: 'rgb' :  field selection requires structure, vector, or interface block on left hand side
ERROR: 0:6: 'assign' : l-value required (can't modify a const)
ERROR: 0:6: 'assign' : cannot convert from 'uniform highp 3-component vector of float' to 'const highp float'

Error
    at new RuntimeError (http://localhost:8080/Build/CesiumUnminified/index.js:9998:11)
    at createAndLinkProgram (http://localhost:8080/Build/CesiumUnminified/index.js:18630:9)
    at reinitialize (http://localhost:8080/Build/CesiumUnminified/index.js:18783:19)
    at initialize2 (http://localhost:8080/Build/CesiumUnminified/index.js:18778:3)
    at ShaderProgram._bind (http://localhost:8080/Build/CesiumUnminified/index.js:18837:3)
    at beginDraw (http://localhost:8080/Build/CesiumUnminified/index.js:30424:17)
    at Context.draw (http://localhost:8080/Build/CesiumUnminified/index.js:30510:3)
    at DrawCommand.execute (http://localhost:8080/Build/CesiumUnminified/index.js:16140:11)
    at DebugInspector.executeDebugShowFrustumsCommand (http://localhost:8080/Build/CesiumUnminified/index.js:192892:16)
    at executeCommand (http://localhost:8080/Build/CesiumUnminified/index.js:193849:27)

@ggetz
Copy link
Contributor

ggetz commented Jan 11, 2023

The label resolution problem does seem to be happening within SDF. Disabling it renders test the same as in main.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 12, 2023

I found a couple stray uses of gl_FragData in CesiumInspector. Replacing these with the appropriate out_FragData_ variable fixes the CesiumInspector Sandcastle.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 12, 2023

For the aliased labels, the problem is here, in BillboardCollectionFS.glsl:

#ifdef GL_OES_standard_derivatives
    float smoothing = fwidth(distance);
    // ...

This extension is not defined in WebGL2 (the derivatives are always available).
If I drop the #ifdef to force that path, the labels are fixed.

However, there are 6 shaders that are doing this #ifdef. We have 2 options:

  1. Replace them all with #if (__VERSION__ == 300 || defined(GL_OES_standard_derivatives))
  2. Drop the check altogether, since according to MDN, the extension in WebGL1 is "universally supported, and can be relied upon to be present"

Option 2 would be a nice (and quick) way to clean up some old code.

@ggetz
Copy link
Contributor

ggetz commented Jan 12, 2023

Drop the check altogether, since according to MDN, the extension in WebGL1 is "universally supported, and can be relied upon to be present"

I'll defer to @lilleyse here if he disagrees with me, but the docs link to https://kdashg.github.io/misc/webgl/webgl-feature-levels.html, which indicates 97% support among webGL users, and we do have checks in place for some of the other extensions on that list. So I would say we should probably be thorough and go with option 1.

@jjhembd
Copy link
Contributor

jjhembd commented Jan 12, 2023

the docs link to https://kdashg.github.io/misc/webgl/webgl-feature-levels.html, which indicates 97% support among webGL users

Thanks @ggetz for the link. I tried tracking down the source of that info—it's from March 2019.

I had trouble finding more recent info. But looking at the visible bars from caniuse.com, the browsers not supporting OES_standard_derivatives are:

  • IE <11
  • Opera Mini and Android Browser <4.4.4 (neither of these support WebGL)
  • UC and QQ browsers (These don't support the WebGL1 extension, but both of them support WebGL2)

Let me know what you think @lilleyse. If needed I can revert the last commit and go with option 1.

@lilleyse
Copy link
Contributor

lilleyse commented Jan 12, 2023

The way I see it there's really no downside to option 1 besides some small code cleanup, so that's my preference.

@ggetz
Copy link
Contributor

ggetz commented Jan 13, 2023

@lilleyse I believe all issue we've identified have been fixed. Ready for merge?

Cartesian4.packFloat(entries[i].height, scratchPackedFloat);
Cartesian4.pack(scratchPackedFloat, heightTexBuffer, i * 4);
}
} else {
heightTexDatatype = PixelDatatype.FLOAT;
heightTexFormat = PixelFormat.LUMINANCE;
heightTexFormat = context.webgl2 ? PixelFormat.RED : PixelFormat.LUMINANCE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Megatexture.js need similar treatment here? It's the only other place I noticed where LUMINANCE and LUMINANCE_ALPHA are being used with FLOAT datatype.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @lilleyse! Yes, it did need a similar fix:

  if (channelCount === 1) {
    pixelFormat = context.webgl2 ? PixelFormat.RED : PixelFormat.LUMINANCE;
  } else if (channelCount === 2) {
    pixelFormat = context.webgl2 ? PixelFormat.RG : PixelFormat.LUMINANCE_ALPHA;

@lilleyse
Copy link
Contributor

Thanks @sanjeetsuhag, @ggetz, and @jjhembd!

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.

Using WebGL 2 by default
6 participants