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

[draft] refactor: remove webgl1 in favor of webgl2 #5165

Closed
wants to merge 2 commits into from

Conversation

0xFA11
Copy link
Contributor

@0xFA11 0xFA11 commented Dec 8, 2024

This is an incomplete draft PR as a follow-up to the WebGL1 removal discussion here #3947

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

const gl =
this._canvas.getContext('webgl2', attributes) as WebGL2RenderingContext ||
this._canvas.getContext('webgl', attributes) as WebGLRenderingContext;
const gl = this._canvas.getContext('webgl2', attributes) as WebGL2RenderingContext;
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 works just fine in browser but we need to also upgrade test infrastructure to WebGL2 supported headless canvas.

Currently, vitest-webgl-canvas-mock package does not come with WebGL2 support out of the box.
It's also somewhat old (last update ~2 years, ~4-5k weekly downloads).
We might consider vitest-canvas-mock as a replacement but I haven't tested whether or not it supports WebGL2 capable headless canvas.
We might also consider skia-canvas as a cairo replacement as a backend but for now, I'm just skipping these topics to isolate and focus on WebGL1 -> WebGL2 transition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only tests that should be using a real canvas are render tests, and they are running in the browser.
The units are basically using mocks in order to run without failing, they don't use the canvas API to draw, so the unit tests should not care about the webgl2 change.

attribute vec2 a_pos;
in vec2 a_pos;
Copy link
Contributor Author

@0xFA11 0xFA11 Dec 8, 2024

Choose a reason for hiding this comment

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

Syntax changes like these should've been made in the original shader code in the first place.
Simply put, they should've been written in WebGL2 syntax and rely on WebGL1 Compact translation code.
If you were to try to run maplibre-gl-js exclusively targeting WebGL2, you'd bump into issues with these shaders written in WebGL1 syntax.
(Maybe we never actually had WebGL2 exclusive tests in order to validate all shaders, but hey, we're here now!)

Rewriting WebGL1 shaders in WebGL2 in the codebase is relatively trivial as shown in this PR.
Changes are simply just attribute to in, gl_FragColor to fragColor in shaders.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you were to try to run maplibre-gl-js exclusively targeting WebGL2, you'd bump into issues with these shaders written in WebGL1 syntax.

How come? WebGL2 fully supports WebGL 1 shading language

Although I agree we should force shaders to be written in GLSL 300

@@ -72,6 +72,7 @@ export class Program<Us extends UniformBindings> {
}

const defines = configuration ? configuration.defines() : [];
defines.unshift('#version 300 es\n');
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 is required to be the first line in WebGL2 shaders, in every shader source code.

Comment on lines -39 to -57
// WebGL1 Compat -- Start

if (type === 'fragment') {
code = code
.replace(/\bin\s/g, 'varying ') // For fragment shaders, replace "in " with "varying "
.replace('out highp vec4 fragColor;', '');
}

if (type === 'vertex') {
code = code
.replace(/\bin\s/g, 'attribute ') // For vertex shaders, replace "in " with "attribute "
.replace(/\bout\s/g, 'varying '); // For vertex shaders, replace "out " with "varying "
}

code = code
.replace(/fragColor/g, 'gl_FragColor')
.replace(/texture\(/g, 'texture2D(');

// WebGL1 Compat -- End
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing WebGL1 Compat translator because we would no longer need to downgrade/rewrite WebGL2 shaders in WebGL1.

@0xFA11 0xFA11 mentioned this pull request Dec 8, 2024
@0xFA11 0xFA11 closed this Dec 8, 2024
@0xFA11 0xFA11 deleted the webgl2 branch December 8, 2024 21:38
@ibesora
Copy link
Collaborator

ibesora commented Dec 11, 2024

This seems like a good starting point on what we'd need to deprecate WebGL1.
I was going to tackle that as part of #5135.
Do you mind me doing that work?

@0xFA11
Copy link
Contributor Author

0xFA11 commented Dec 12, 2024

@ibesora — I think we wouldn't completely remove WebGL1 support but instead, implement a WebGL2 to WebGL1 fallback depending on what's available at runtime on the canvas, rather than a complete WebGL1 removal.

there are some conversations and context here #5166 and here #3947

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