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

Revise step interpolation for lines, increase precision of line progress as needed #9694

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

karimnaaji
Copy link
Contributor

@karimnaaji karimnaaji commented May 15, 2020

Refer #9682.

A nav native use case requires hard transition between different color stops while using the line-gradient property. We currently do not provide the best result for their use case. This PR is an attempt to reduce the limitations from this use case by increasing the precision and switching to a more appropriate texture sampler for the step property:

Produces discrete, stepped results by evaluating a piecewise-constant function defined by pairs of input and output values ("stops")

Changes

  • Evaluate worst scenario coverage of line string and increase texture size resolution accordingly, by making sure we are not going over hardware size limits.
  • Change the sampling to nearest: to maximize the results under most circumstances, the sampling is changed from linear to nearest for step interpolant. Without taking into consideration the other precision limitations, this gives better results overall.
  • To limitate memory issues on their use case, reuse the memory placement of the color ramp if it was already created (also done for heatmaps). This allows to update the expression of the color ramp gradient without involving reallocation for memory that's already there: 13779a5
  • Increase the precision of line progress when we pass some threshold known to lack precision 2c7ad46. The memory cost is not involved in the common case.
  • Tile line gradient textures and render them between the line clips of the current tile, this means the texture is now per tile instead of per layer. This was the best approach to achieve maximum accuracy across zoom levels and long routes.

Differences from the above changes with master

actual
expected
diff

Approach comparisons

(1) Instead using the texture to encode the color ramp, encode the expression stop in the texture and use glsl functions to evaluate in the shader.
The advantage of this approach would be to reduce the texture memory being used under this use case. The disavantage is the dependency on the expression and the shader code that would be interoduced. Here is how it would look visually (using the step glsl function): step-glsl.mov.zip
The results are not different enough from (2) to warrant the complexity.
(2) Using nearest with a very large texture (the approach of this PR): 16k-texture-nearest.mov.zip
(3) Using a very large texture while keeping linear as a texture sampler: 16k-texture-linear.mov.zip

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • [n/a] document any changes to public APIs
  • [n/a] post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team for visual changes
  • tagged @mapbox/gl-native for native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Vastly increased precision of line-gradient for long route coverage, revise step interpolant to result in harder visual transition between stops</changelog>
  • Fix issue with gradient textures invalidation (754be74)

@karimnaaji karimnaaji requested a review from ansis May 15, 2020 19:18
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

I've left a comment about correctly detecting step interpolation for line-gradient properties.

The only other question I have is if this is stable at high zooms with very long routes, say cross-continental. If so, would it be possible to make a regression test to ensure that it can be incremented accurately as progress is made along a line/route ?

@sgolbabaei sgolbabaei added this to the release-e milestone May 20, 2020
@karimnaaji
Copy link
Contributor Author

karimnaaji commented May 21, 2020

The only other question I have is if this is stable at high zooms with very long routes, say cross-continental

We're still limited for cross-continental route, and it's relatively not stable at high zooms. For that type of route, at zoom 18, we're trying to represent too many pixels with the current architecture so it would start to break because of that. This goes unnoticed with gradients but is not forgiving for a precise interpolant like step. We may also suffer jumps because line progress is encoded in 15 bits.

To support the long route case, a better approach would be to build a different vertex layout in line buckets, have different colors as part of the vertex layout, and inject splits as we evaluate the expression while building the line.

example

where vertices v0..v3 are given a color attribute, v4..v7 another, and the split (@ v2/v4) happens as the step expression is evaluated. I think we should give the above a try after we mitigate the issue with this proposal, it's more involved and we should be careful to not degrade the most common case.

For mid-long routes, I have a test that accounts for a rather long distance (before we start hitting precision issues).

test/integration/render-tests/line-gradient/gradient-step-zoomed/style.json

The geometry used was the following:

Screen Shot 2020-05-20 at 4 04 17 PM

@karimnaaji
Copy link
Contributor Author

Something I just thought about for very long routes is to increase the precision of v_lineprogress and create a second vertex buffer specifically for it, where it wouldn't be encoded in 15bits but either f64/32. This has a memory cost, but by having it in a separate vertex buffer, we'd allow to hit that cost only on cases where incoming geometry to be represented is known to not be enough. Worth trying for this PR!

@karimnaaji
Copy link
Contributor Author

karimnaaji commented May 21, 2020

Follow up of this PR implemented in 03262f1 for support of long routes. I haven't merge it in here now as we can't introduce shader related changes if we'd like to have that ported in gl-native in a reasonable amount of time (dependency on other port before we can update the submodule there).

EDIT: merged in this PR as it is a strict requirement from nav-native.

@karimnaaji karimnaaji changed the title Revise step interpolation for lines Revise step interpolation for lines, increase precision of line progress as needed May 27, 2020
@karimnaaji
Copy link
Contributor Author

karimnaaji commented May 27, 2020

@ansis Some comparisons and example data on a cross continental route:

new z4
new-z4
old z10
master-z4

new z10
new-z10
old z10
master-z10

Videos for full frames (degraded by gifs):
videos.zip

Patch used for the above
diff --git a/debug/line-gradient.html b/debug/line-gradient.html
index 89b443e35..fccd06a1d 100644
--- a/debug/line-gradient.html
+++ b/debug/line-gradient.html
@@ -31,7 +31,7 @@
             "type": "Feature",
             "properties": {},
             "geometry": {
-                "coordinates": [[-77.044211, 38.852924 ], [-77.045659, 38.860158 ], [-77.044232, 38.862326 ], [-77.040879, 38.865454 ], [-77.039936, 38.867698 ], [-77.040338, 38.86943 ], [-77.04264, 38.872528 ], [-77.03696, 38.878424 ], [-77.032309, 38.87937 ], [-77.030056, 38.880945 ], [-77.027645, 38.881779 ], [-77.026946, 38.882645 ], [-77.026942, 38.885502 ], [-77.028054, 38.887449 ], [-77.02806, 38.892088 ], [-77.03364, 38.892108 ], [-77.033643, 38.899926 ] ],
+                "coordinates": [[-120.044211, 38.852924 ], [-77.045659, 38.860158 ] ],
                 "type": "LineString"
             }
         }]
@@ -46,6 +46,7 @@
             data: gj
         });
 
+        const routeCompleted = 0.3;
         map.addLayer({
             type: 'line',
             source: 'line',
@@ -54,22 +55,27 @@
                 'line-color': 'red',
                 'line-width': 14,
                 'line-gradient': [
-                    'interpolate',
-                    ['linear'],
+                    'step',
                     ['line-progress'],
-                    0, "rgba(0, 0, 255, 0)",
-                    0.1, "royalblue",
-                    0.3, "cyan",
-                    0.5, "lime",
-                    0.7, "yellow",
-                    1, "red"
+                    "rgba(0, 0, 255, 0.1)",
+                    1.0, "yellow"
                 ]
             },
-            layout: {
-                'line-cap': 'round',
-                'line-join': 'round'
-            }
         });
+
+        setInterval(function () {}, 1000);
+        let offset = 0.001;
+        function animate(timestamp) {
+            map.setPaintProperty('line', 'line-gradient',  [
+                    'step',
+                    ['line-progress'],
+                    "rgba(0, 0, 255, 0.1)",
+                    Math.cos(offset) * 0.5 + 0.5, "yellow"
+                ]);
+            offset += 0.001;
+            requestAnimationFrame(animate);
+        }
+        animate(0);
     });
 
 </script>

@asheemmamoowala
Copy link
Contributor

asheemmamoowala commented May 28, 2020

@karimnaaji What is the max precision possible after these changes? I find that the step expression supports a precision of ~0.0001. The outcome is that on the cross-continental route line described in #9694 (comment), the max step of the line gradient is still very large, meaning that one wouldn't be able to use this expression to update a car's position on a long route. To simulate this, I tried a pitched map at z16-z18 to see if I could step the gradient at a granular level.

@karimnaaji
Copy link
Contributor Author

I find that the step expression supports a precision of ~0.0001

That sounds about right. To clarify with some numbers what you're experiencing: at z18, from east to west (north america), that's ~8k tiles that we try to represent with 4 096 000 px at 512 px a tile, so we need changes w/ 2.44140625e-7 accuracy; which we can now represent in uv space with an f32 in 2c7ad46, but depending on the hardware max texture size, we hit a bound between 0.00024414062 to 0.00006103515 for that specific case... We would need a texture of about 4096*1024, (2.38418579e-7 accuracy) to match the line progress, that's 4mb of data.

Two more ideas:
(1) Increase texture size even more and wrap texture at line ending: uses more texture space and wouldn't get us that far ahead in precision, a lot more data to update, and more memory involved (the 4mb mentioned above): I have performance concerns with that, we shouldn't do it.
(2) Tile those textures, that means we render them partially between the line clips, while adjusting the uvs based on those: one issue to overcome is that the clips are per line and multiple lines can be drawn in the same draw call. But this should get us to the tile unit accuracy.

@karimnaaji
Copy link
Contributor Author

(2) implemented in 9d73d1a, but needs a last optimization and cleanup pass.

@karimnaaji
Copy link
Contributor Author

@ansis We're done hunting for better precision, please have a look.

@karimnaaji karimnaaji force-pushed the karim/color-ramp-step branch 2 times, most recently from 421c734 to 754be74 Compare June 1, 2020 16:56
@karimnaaji karimnaaji force-pushed the karim/color-ramp-step branch 3 times, most recently from 56b1542 to 8238f98 Compare June 2, 2020 00:46
karimnaaji added a commit that referenced this pull request Jun 2, 2020
Increase texture resolution for line gradient for step interpolant
- Step interpolant now uses gl.NEAREST for texture sampler: This allows
for hard transition between different line sections when using step
interpolation. Other use cases are covered by using a smooth interpolation
type along with gl.LINEAR texture sampler.
- Step interpolant now uses an increased texture resolution color ramp
based on the total line length tile coverage to limitate the potential
precision issues for high coverage. We are still limitated by the precision
of v_lineprogress currently encoded in a 15bits floating point value.

Reuse color ramp memory placement when already available

Only calculate max distance if clipstart and clipend are defined

Increase precision of line progress when needed

Tile line gradient textures
karimnaaji added a commit to mapbox/mapbox-gl-native that referenced this pull request Jun 2, 2020
karimnaaji added a commit to mapbox/mapbox-gl-native that referenced this pull request Jun 2, 2020
karimnaaji added a commit to mapbox/mapbox-gl-native that referenced this pull request Jun 3, 2020
vec4 color = texture2D(u_image, vec2(v_lineprogress, 0.5));
highp float texel_height = 1.0 / u_image_height;
highp float half_texel_height = 0.5 * texel_height;
highp vec2 uv = vec2(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this introduces a dependent texture read (prevents prefetch), most es 3.0 capable hardware won't take the hit but it's worth profiling.

// Constructs a second vertex buffer with higher precision line progress
if (this.lineClips) {
this.layoutVertexArray2.emplaceBack(
this.scaledDistance - this.lineClips.start,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By always realigning the distance with clip start and rendering the gradient between start and end while tiling it, we allow to have way larger distances represented, this is where the increase in precision comes from.

The last parameter of this new attribute layout is used to select which line the gradient falls through when multiple lines are represented in the same bucket geometry.

@karimnaaji
Copy link
Contributor Author

karimnaaji commented Jun 3, 2020

@ansis PTAL and let me know if anything would help or should be made more clear to facilitate the review. Note that the focus is on precision increase, we may want to address any other potential improvements in separate patches as this already gets us a step forward.

mourner
mourner previously requested changes Jun 3, 2020
src/util/util.js Outdated Show resolved Hide resolved
src/util/util.js Outdated Show resolved Hide resolved
src/util/color_ramp.js Outdated Show resolved Hide resolved
@karimnaaji karimnaaji requested a review from mourner June 3, 2020 22:56
karimnaaji added a commit to mapbox/mapbox-gl-native that referenced this pull request Jun 4, 2020
karimnaaji added a commit to mapbox/mapbox-gl-native that referenced this pull request Jun 4, 2020
- Step interpolant now uses gl.NEAREST for texture sampler: This allows
for hard transition between different line sections when using step
interpolation. Other use cases are covered by using a smooth interpolation
type along with gl.LINEAR texture sampler.
- Step interpolant now uses an increased texture resolution color ramp
based on the total line length tile coverage to limitate the potential
precision issues for high coverage. We are still limitated by the precision
of v_lineprogress currently encoded in a 15bits floating point value.

Reuse color ramp memory placement when already available

Only calculate max distance if clipstart and clipend are defined

Increase precision of line progress when needed

Tile line gradient textures

Review comments

- Eliminate Math.log2 (not available on ie11)
- Inline and compress evaluation interpolations for line clip by
specializing it for the use case

Thanks @mourner
- Use single attribute for uv on x, move computation cpu side
- Offload fragment shader from a few operations by moving it to vertex
- More explicit addHalfVertex function block
Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks great

@karimnaaji karimnaaji dismissed mourner’s stale review June 9, 2020 16:18

Addressed changes

@karimnaaji karimnaaji merged commit 58a7390 into master Jun 9, 2020
@karimnaaji karimnaaji deleted the karim/color-ramp-step branch June 9, 2020 16:31
karimnaaji added a commit that referenced this pull request Jun 9, 2020
…ess as needed (#9694)

* Vastly increase precision for line gradients
- Step interpolant now uses gl.NEAREST for texture sampler: This allows
for hard transition between different line sections when using step
interpolation. Other use cases are covered by using a smooth interpolation
type along with gl.LINEAR texture sampler.
- Step interpolant now uses an increased texture resolution color ramp
based on the total line length tile coverage to limitate the potential
precision issues for high coverage
- Reuse color ramp memory placement when already available
- Precision is increased by two factors for geojson with line metrics on:
  - Tile line gradient textures for higher representation in texture space
  - Add an optional extension vertex buffer for increased line progress precision

* Review comments (Thanks @mourner)
- Eliminate Math.log2 (not available on ie11)
- Inline and compress evaluation interpolations for line clip by
specializing it for the use case

* Optimization pass
- Use single attribute for uv on x, move computation cpu side (Thanks @ansis)
- Offload fragment shader from a few operations by moving it to vertex
- More explicit addHalfVertex function block
rokuz pushed a commit that referenced this pull request Jun 24, 2020
…ess as needed (#9694)

* Vastly increase precision for line gradients
- Step interpolant now uses gl.NEAREST for texture sampler: This allows
for hard transition between different line sections when using step
interpolation. Other use cases are covered by using a smooth interpolation
type along with gl.LINEAR texture sampler.
- Step interpolant now uses an increased texture resolution color ramp
based on the total line length tile coverage to limitate the potential
precision issues for high coverage
- Reuse color ramp memory placement when already available
- Precision is increased by two factors for geojson with line metrics on:
  - Tile line gradient textures for higher representation in texture space
  - Add an optional extension vertex buffer for increased line progress precision

* Review comments (Thanks @mourner)
- Eliminate Math.log2 (not available on ie11)
- Inline and compress evaluation interpolations for line clip by
specializing it for the use case

* Optimization pass
- Use single attribute for uv on x, move computation cpu side (Thanks @ansis)
- Offload fragment shader from a few operations by moving it to vertex
- More explicit addHalfVertex function block
rokuz pushed a commit that referenced this pull request Jun 25, 2020
…ess as needed (#9694)

* Vastly increase precision for line gradients
- Step interpolant now uses gl.NEAREST for texture sampler: This allows
for hard transition between different line sections when using step
interpolation. Other use cases are covered by using a smooth interpolation
type along with gl.LINEAR texture sampler.
- Step interpolant now uses an increased texture resolution color ramp
based on the total line length tile coverage to limitate the potential
precision issues for high coverage
- Reuse color ramp memory placement when already available
- Precision is increased by two factors for geojson with line metrics on:
  - Tile line gradient textures for higher representation in texture space
  - Add an optional extension vertex buffer for increased line progress precision

* Review comments (Thanks @mourner)
- Eliminate Math.log2 (not available on ie11)
- Inline and compress evaluation interpolations for line clip by
specializing it for the use case

* Optimization pass
- Use single attribute for uv on x, move computation cpu side (Thanks @ansis)
- Offload fragment shader from a few operations by moving it to vertex
- More explicit addHalfVertex function block
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.

6 participants