Fix color style for WebGL tile layer

[kikuchan]

This PR moves alpha premultiplication from the WebGL expression compiler to styleparser.

This change is necessary because alpha premultiplication strategies can differ between shaders.
The shader in WebGLTileLayer employs a lazy strategy, while the shader in styleparser uses an eager strategy.
Therefore, it is more appropriate to avoid applying alpha premultiplication within the color operator.

Fixes #16074.

[github-actions]

📦 Preview the website for this branch here: https://deploy-preview-16075--ol-site.netlify.app/.

[jahow]

Thanks for this PR @kikuchan, after trying it out a little locally I think it makes sense. I honestly can't remember why I added alpha premultiplication in the colorToGlsl function back then, I mean it works but it seems way more straightforward to do this premultiplication on the final color in the shader.

I'm leaving a few comments that will hopefully make it simpler in the end, and then we can see if the rendering tests still pass! thanks!

[jahow]

I'm wondering if we can just get rid of this samplePremultiplied function altogether now

[jahow]

I think these should go in the final shader outputs in ShaderBuilder.js:

• for symbols:

@@ -582,7 +582,7 @@ void main(void) {
   float c = cos(v_angle);
   float s = sin(v_angle);
   coordsPx = vec2(c * coordsPx.x - s * coordsPx.y, s * coordsPx.x + c * coordsPx.y);
-  gl_FragColor = ${this.symbolColorExpression_};
+  gl_FragColor = premultiplyAlpha(${this.symbolColorExpression_});
   if (u_hitDetection > 0) {
     if (gl_FragColor.a < 0.05) { discard; };
     gl_FragColor = v_prop_hitColor;

• for strokes:

@@ -846,7 +841,7 @@ void main(void) {
   float currentLengthPx = max(0., min(dot(segmentTangent, startToPoint), segmentLength)) + v_distanceOffsetPx; 
   float currentRadiusPx = abs(dot(segmentNormal, startToPoint));
   float currentRadiusRatio = dot(segmentNormal, startToPoint) * 2. / v_width;
-  vec4 color = ${this.strokeColorExpression_} * u_globalAlpha;
+  vec4 color = ${this.strokeColorExpression_};
   float capType = ${this.strokeCapExpression_};
   float joinType = ${this.strokeJoinExpression_};
   float segmentStartDistance = computeSegmentPointDistance(currentPoint, v_segmentStart, v_segmentEnd, v_width, v_angleStart, capType, joinType);
@@ -856,7 +851,7 @@ void main(void) {
     max(segmentStartDistance, segmentEndDistance)
   );
   distance = max(distance, ${this.strokeDistanceFieldExpression_});
-  gl_FragColor = color * smoothstep(0.5, -0.5, distance);
+  gl_FragColor = premultiplyAlpha(color) * smoothstep(0.5, -0.5, distance) * u_globalAlpha;
   if (u_hitDetection > 0) {
     if (gl_FragColor.a < 0.1) { discard; };
     gl_FragColor = v_prop_hitColor;

• for fills:

@@ -954,7 +954,7 @@ void main(void) {
   }
   #endif
   if (${this.discardExpression_}) { discard; }
-  gl_FragColor = ${this.fillColorExpression_} * u_globalAlpha;
+  gl_FragColor = premultiplyAlpha(${this.fillColorExpression_}) * u_globalAlpha;
   if (u_hitDetection > 0) {
     if (gl_FragColor.a < 0.1) { discard; };
     gl_FragColor = v_prop_hitColor;

This way premultiplyAlpha doesn't have to be called in too many places.

Note: I've tried these changes locally but it still broke the rendering... maybe I'm missing something, but I feel like this should work somehow.

[jahow]

same as above

[jahow]

same as above

[jahow]

I think we can leave this call out, see my comments on ShaderBuilder

[jahow]

same as above

[jahow]

same as above

[jahow]

same as above

[jahow]

same as above

[jahow]

same as above

[kikuchan]

Hi @jahow, thank you for your review and suggestions!

Unfortunately, I tried rewriting the styleparser, but it didn't work because of the mix function.
The function doesn't work well with regular alpha blendings as you know, and I think that's why you implemented the current method previously.
Furthermore, since vector tile styles allow users to use interpolate operator, color values should be premultiplied by their alpha value before the operator is applied.

To address the issue of multiple of premultiplyAlpha, I've implemented a new approach: introducing a premultiplyAlpha context flag that tells the compiler whether or not to perform premultiplication.
Additionally, this fixes the array, interpolate, and some other operators within the color context by applying premultiplications as soon as possible during compilation.

What are your thoughts on this approach?
Please let me know, and I'm very happy to change my code!

Thanks!

[jahow]

Ok, I think there is a way to make the alpha premultiplication much less of a concern throughout the code. See my branch here: https://github.com/jahow/openlayers/tree/fix-webgl-color-style

Alpha premultiplication is now only done at the end of each fragment shader. I kept all your changes throught the code to not multiply everything by alpha all the time, and also added some changes in the style parser to adapt the color expressions accordingly. The rendering tests pass, so it's a matter of adapting the unit tests now. What do you think? I feel like this would be the bast way forward.

Thank you for your continued efforts!

[kikuchan]

Oh! It surely works! It's great!
I'm sorry I was wrong, I was missing some alpha premultiplications that need to be avoided in my WIP code.
Thank you for your code! It's been very helpful to compare it with mine.

I've sent a rebased and revised version.
Also, I added you as a 'Co-authored-by' in the commit message.

Thank you!

[kikuchan]

BTW, currently u_globalAlpha is not applied here. Is this OK for a symbol...?

[jahow]

Oh, right, maybe that was missing. I think this should be done in another PR, I have the feeling this isn't tested anywhere

[jahow]

That looks perfect, thank you for sticking to this!!