Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Composite functions behave incorrectly with non-integer stops #8460

Closed
lbud opened this issue Mar 17, 2017 · 3 comments
Closed

Composite functions behave incorrectly with non-integer stops #8460

lbud opened this issue Mar 17, 2017 · 3 comments
Assignees
Labels
bug Core The cross-platform C++ core, aka mbgl

Comments

@lbud
Copy link
Contributor

lbud commented Mar 17, 2017

Composite ("zoom-and-property") functions behave differently in mapbox-gl-native and mapbox-gl-js: part of this is intentional and part is a bug.

Considering the following function:

"fill-extrusion-height": {
    "type": "exponential",
    "property": "height",
    "stops": [
        [{ "zoom": 15, "value": 0 }, 0],
        [{ "zoom": 15, "value": 600 }, 0],
        [{ "zoom": 15.2, "value": 0 }, 0],
        [{ "zoom": 15.2, "value": 600 },600]
    ]
}

In mapbox-gl-js, this would cause fill-extrusions to rise from flat at z15 to property-based heights at z15.2, and then maintain that height at all higher zooms.

In mapbox-gl-native, since PaintPropertyBinders are evaluated at layout time at integer zoom levels, we won't be able to assign different slopes to z15-15.2 and 15.2-16, and it will, necessarily, behave differently.

However, current behavior is as such:

  • from z15 to z15.2, fill-extrusions grow from flat to identity heights, as expected
  • from z15.2 to z16 they continue to grow upward along that same slope and become 5x too high
  • at z16 and onward they reset to flat

When I add another set of stops,

[{ "zoom": 22, "value": 0 }, 0],
[{ "zoom": 22, "value": 600 }, 600]

the above behavior only changes in that at z16 they reset to their intended property-based identity height and maintain that height to z22.

This is an issue with the logic in coveringRanges. I believe what needs to happen is:

  • the upper_bound line should look for zoom + 1
  • if a stop chosen as one of the bounds is outside of the range of this integer zoom (n-n.99), and there is another stop between said outer stop and the other bounds stop, since we know that inside stop would otherwise be ignored altogether, then we need to insert an artificial stop at the exact outer bound of the zoom and, for its values, interpolate between the chosen outer bounds stop and the inner (otherwise ignored) stop.

The following shows the behavior with the above function for a feature with value=600: pink is how mapbox-gl-js behaves (and is the literal interpretation of the function stops), green is how mbgl-native currently works, yellow is how mbgl-native would work if only changing the upper_bound here to (zoom + 1), and blue is how I believe it should work, with artificial stops.
mbgl-native-compositefn-1

Same below, but for a function specified as

"fill-extrusion-height": {
    "type": "exponential",
    "property": "height",
    "stops": [
        [{ "zoom": 15, "value": 0 }, 0],
        [{ "zoom": 15, "value": 600 }, 0],
        [{ "zoom": 15.2, "value": 0 }, 0],
        [{ "zoom": 15.2, "value": 600 },300],
        [{ "zoom": 22, "value": 0 }, 0],
        [{ "zoom": 22, "value": 600 }, 600]
    ]
}

mbgl-native-compositefn-2

@lbud lbud added bug Core The cross-platform C++ core, aka mbgl labels Mar 17, 2017
@lbud lbud mentioned this issue Mar 17, 2017
12 tasks
@jfirebaugh
Copy link
Contributor

Awesome graphs and analysis!

Thinking through one more thing here: for these cases where there's only a single stop in the given integer zoom range, could native get behavior identical to the gl-js behavior by changing how it calculates the interpolation factor?

For example, with the first example, we could define the interpolation factor f such that f(z) = 0 for z <= 15, f(z) = 1 for z >= 15.2, and linearly interpolated in between. Then the shader would "do the right thing" when it uses f(z) to interpolate between the min and max values in the layout vertex attribute, and we'd get the magenta line in the graph.

In the second example, it seems like this breaks down though. We can say f(z) = 0 for z <= 15, and f(16) = 1, but how do we define f for 15 < z < 16 and z > 16 (which we need because the tile may be rendered with overzoom)? The slope between 15 and 15.2 and between 15.2 and 16, depend on the function result at 15.2 for a particular source property value. But we only have one interpolation factor uniform, which must have the same value for a given z across all source features.

So, my conclusion is that native can match gl-js exactly in the first example, but must use the approximation in blue in the second. Does that sound right?

@anandthakker
Copy link
Contributor

Let me just throw in another 🐒 🔧 here.

Over in mapbox/mapbox-gl-js#4228, one thing I was working through is that, when text-size is a composite function, the shader is actually going to need the value of that function evaluated at two zoom levels: a) the current zoom at which we're rendering the map, and b) 1 + tile.zoom, where tile.zoom is the zoom level of the vector tile itself. [1]

Rather than wasting buffer space / attribute bindings on two different values of the same function, I have been thinking that it might be worth trying to have the shader calculate both of these values based on the stop values given to it. (I.e., we'd give it an additional interpolation_t value in order to calculate (b).).

Not sure if this will end up being the best path, but even if not (and certainly if so!), I think we may want to consider bringing JS and native's composite-function calculations into sync by sticking with just 2 stops in JS.

[1] The reason for this is because there's a bunch of layout data that's constructed using b), and the shader will thus need both values to make adjustments.

@ericrwolfe
Copy link
Contributor

Just ran into this with a style created in Studio:

For two symbol layers, icon-opacity and text-opacity were using composite functions with zoom level stops 16.99 and 17 which would lead to the following whiteout/blackout rendering bug on freshly loaded versions of the style:

pasted image at 2017_05_31 10_42 am

Changing the zoom levels to an integer (16 instead of 16.99) would resolve the issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

No branches or pull requests

4 participants