Combine line shaders #10894

endanke · 2021-07-26T14:09:28Z

This PR combines the dash array, gradient, and regular line style related shaders. It will make the long-term maintenance easier and also enable the combined usage of dash array with gradient fills.

I've purposefully kept the separate shader for the line-pattern property, because currently we don't have a use-case for it's combined usage with the other types. It has also a bit more different fragment shader that the other 3 variations.

Visual changes: See the newly added render test expectation for the combined usage of line-gradient and line-dasharray.

API changes: This PR will remove the Disabled by line-dasharray requirement from the line-gradient property.

GL-Native: I'll implement the native port.

Launch Checklist

@mapbox/map-design-team @mapbox/static-apis

…sharray

karimnaaji

Very nice! Left a few questions but nothing major, glad to see such a consolidation.

karimnaaji · 2021-07-27T11:48:18Z

src/shaders/line.vertex.glsl

+    #pragma mapbox: initialize lowp float floorwidth
+    #pragma mapbox: initialize lowp vec4 dash_from
+    #pragma mapbox: initialize lowp vec4 dash_to


Is there any risk of crossing the GL_MAX_VERTEX_ATTRIB for low-end devices for native? I recall that a common denominator for es 2 is 8 (safe to use). We are at 9 from this PR, I'm not sure if after this change all of these #pragmas may be used at once but there might be ways to consolidate them if that's a possibility. (Refer https://chromium.googlesource.com/angle/angle/+/HEAD/doc/ResourceLimits.md.)

This is a really good point! ~~I was thinking to wrap these too with #ifdefs but it had some issues, maybe this pragma is evaluated before them? I'll check how else could I wrap these.~~
Edit: I realized wrapping doesn't matter because we need to consider the edge case where all of the attributes are used.

Seems very important to respect these limits, but just out of curiosity: since this PR consolidates but does not change the functionality, it should be possible to overcome this with judicious ifdef's or similar, right?

I've looked into this a bit more and noticed that the shader for the dash line had also more than 8 attributes. It could be an issue on old devices, but since it was already implemented like that, this PR probably won't cause more issues.

If I understand it correctly #pragma mapbox will only produce attributes when the property is calculated. So with the attributes above, the cases when there could be more than 8 only happens when both gradient and dash are enabled, and at least 4 properties are calculated. Otherwise I think it should work fine.
For those cases maybe it would be good to implement a validation which was discussed here: #4879

@rreusser I think yes but also this PR enables the combined usage of dash and gradients, so technically it extends the scope of the shader a bit and might use more attributes when both of them are used at the same time.

Maybe for now dash and gradient combined usage could be still disabled. On older devices it could be done with two passes to overcome the limitation, which could be implemented later.

And another possible workaround: maybe I could pack a_linesofar, a_uv_x, and a_split_index into a vec3 and upload them as a single attribute. The downside is that they need an extra operation to unpack them, but this way we would have the same amount of attributes as we had before in line_sdf.vertex.glsl.
What do you think?

Edit: I've pushed up the changes for this idea.

karimnaaji · 2021-07-27T11:51:53Z

src/render/program/line_program.js

+    const values = [];
+    if (hasDash(layer)) values.push('RENDER_LINE_DASH');
+    if (layer.paint.get('line-gradient')) values.push('RENDER_LINE_GRADIENT');
+    return values;


Not a blocker for this PR and could be deferred, but wondering if these new shader variants are still compiled as part of the shader pre-compilation step (introduced by commit b93d709), if it's not we should add a follow up ticket to look at that.

I've checked it now and I think it does. At least in painter.js I can see the program in the cache and the keying also depends on these define values. (I've actually written this based on the circleDefinesValues which are used in a similar way.)

Thanks for checking that one! That makes sense.

endanke · 2021-07-27T14:20:32Z

@astojilj also noted that it would be better for perfomance to keep the line uniform values separated in line_program.js.
Currently I didn't do that because I noticed that the cached shaders are loading the uniforms based on the program's name, and it would need a bit more refactoring to identify the program variations based on the defines. It seems there are some other shaders which use some of the uniforms conditionally, I think it could be fixed as part of another ticket which considers them all.

karimnaaji

LGTM, thanks for looking at the attribute limit.

mourner · 2021-07-30T19:48:21Z

@endanke can you share a benchmark run result for this PR (comparing this and the previous commit) (using benchmap-js), just to make sure there are no unexpected performance regressions?

endanke · 2021-08-02T08:34:15Z

@mourner Thanks for the note, I was just wondering which tool should we use for the benchmarks!
I'll post the results after I get the required access level to execute the job.

endanke · 2021-08-03T17:23:14Z

@mourner attached the test results

rreusser · 2021-08-03T18:28:12Z

Looks great! 👏 👏 Just a quick check: if this PR did have a detrimental effect on performance, would we expect it to show up in the benchmarks? From what I understand, we gauge two things: 1) js load time (this PR saves some bytes, but I guess not enough to show up), and 2) speed index, which scores based on how long it takes pixels to reach their final color (which I suspect may be limited by tile loading and bucket processing and perhaps not by GPU performance)

An idea: once the map loads and we're all done computing load time and speed index, maybe we could set repaint = true and use EXT_disjoint_timer_query to measure the rendering performance.

I love the benchmarks; I just want to be careful not to read into them something that we're not actually measuring.

cc @ansis

endanke · 2021-08-04T12:16:42Z

@rreusser fair point and I agree it's a good idea to measure the GPU time specifically.
I did a naive manual test using your suggestion, by adding the following snippet to debug/line-gradient.html:

const frameLimit = 300;
var frameCounter = 0;
var gpuTimeSum = 0;
map.repaint = true;
map.on('gpu-timing-frame', (result) => {
    if (frameCounter < frameLimit) {
        gpuTimeSum += result.gpuTime;
        frameCounter++;
    } else {
        console.log("Avg GPU time: ", gpuTimeSum / frameLimit);
    }
});

I've ran it 5 times for both commits using Google Chrome 92.0.4515.107. The results on average are:

2.0626013333 (after changes)
1.9422386667 (before changes)

As I understand this metric is in nanoseconds, so the change doesn't seem too drastic. Of course it's a very specific environment, so it would be better to run something similar in our test environment.

rreusser · 2021-08-04T16:45:38Z

Thanks for testing that out! 👏 I know I'm being pedantic, but I'm admittedly trying to help make sure we're all very clear on what the benchmarks do and don't measure. I suppose what the above benchmarks tell us is that the change in bytes loaded and… probably mostly shader compilation doesn't impact performance. As far as the GPU goes, I guess the number of line fragments will probably always be so small that it's unlikely to impact overall rendering even if we really try to accurately measure the difference. (This makes me think I should benchmark fog since that probably does have a measurable impact on rendering)

endanke · 2021-08-04T17:24:19Z

@rreusser no worries at all, I agree it's important to evaluate these changes! I'll also make some further tests in GL-Native, including a case where a large amount of lines are rendered at the same time. Maybe we could use similar stress tests for different layer types as well.

endanke force-pushed the endanke/combined-line-shaders branch 3 times, most recently from 3477b5d to 3a90a46 Compare July 27, 2021 08:45

endanke added the feature 🍏 label Jul 27, 2021

endanke marked this pull request as ready for review July 27, 2021 11:06

endanke changed the title ~~[WIP] Combine line shaders~~ Combine line shaders Jul 27, 2021

endanke added 8 commits July 27, 2021 14:28

Move dash and gradient line rendering to line program

294bd05

Remove LineSDF and LineGradient sources

ef7ebb4

Remove unused variables, fix linter errors

276c0a4

Remove unused uniforms

8c31313

Fix type usage

7ae3246

Check for computed dash array values in line program

39faedf

Add render test case for combined usage of line-gradient with line-da…

8b71ea2

…sharray

Remove negated line-dasharray requirement from line-gradient

3ac84fa

endanke force-pushed the endanke/combined-line-shaders branch from 13afd42 to 3ac84fa Compare July 27, 2021 11:28

karimnaaji reviewed Jul 27, 2021

View reviewed changes

endanke force-pushed the endanke/combined-line-shaders branch from d1aafc6 to f9e8e73 Compare July 27, 2021 13:25

Make gradient attributes packed

0c5b453

endanke force-pushed the endanke/combined-line-shaders branch from f9e8e73 to 0c5b453 Compare July 28, 2021 07:46

karimnaaji approved these changes Jul 28, 2021

View reviewed changes

endanke merged commit 54615bd into main Jul 28, 2021

endanke deleted the endanke/combined-line-shaders branch July 28, 2021 09:12

karimnaaji added this to the v2.5 milestone Sep 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine line shaders #10894

Combine line shaders #10894

endanke commented Jul 26, 2021 •

edited

karimnaaji left a comment

karimnaaji Jul 27, 2021

endanke Jul 27, 2021 •

edited

rreusser Jul 27, 2021

endanke Jul 27, 2021

endanke Jul 27, 2021

endanke Jul 27, 2021

endanke Jul 28, 2021 •

edited

karimnaaji Jul 27, 2021

endanke Jul 27, 2021

karimnaaji Jul 27, 2021

endanke commented Jul 27, 2021

karimnaaji left a comment

mourner commented Jul 30, 2021

endanke commented Aug 2, 2021

endanke commented Aug 3, 2021 •

edited

rreusser commented Aug 3, 2021 •

edited

endanke commented Aug 4, 2021

rreusser commented Aug 4, 2021

endanke commented Aug 4, 2021

Combine line shaders #10894

Combine line shaders #10894

Conversation

endanke commented Jul 26, 2021 • edited

Launch Checklist

karimnaaji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endanke Jul 27, 2021 • edited

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

endanke Jul 28, 2021 • edited

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

endanke commented Jul 27, 2021

karimnaaji left a comment

Choose a reason for hiding this comment

mourner commented Jul 30, 2021

endanke commented Aug 2, 2021

endanke commented Aug 3, 2021 • edited

rreusser commented Aug 3, 2021 • edited

endanke commented Aug 4, 2021

rreusser commented Aug 4, 2021

endanke commented Aug 4, 2021

endanke commented Jul 26, 2021 •

edited

endanke Jul 27, 2021 •

edited

endanke Jul 28, 2021 •

edited

endanke commented Aug 3, 2021 •

edited

rreusser commented Aug 3, 2021 •

edited