Clone stylesheet to allow toggling different styles with `setStyle` #11942

avpeery · 2022-05-27T00:58:41Z

Closes #11939 and #11916

Previously, we would overwrite the stylesheet. However, this prevents successfully toggling between two styles using setStyle as it overwrites some of the properties. See both issues above in which terrain and projections are overwritten.

In addition, updating a terrain source did not account for using default values in the style. Currently, exaggeration is the only property of terrain with a default value.

Before (see codepen for example, toggling style changes the stylesheet exaggeration of style a):

Screen.Recording.2022-05-26.at.8.56.02.PM.mov

After (style exaggeration is not impacted, toggling is successful between style a and style b):

Screen.Recording.2022-05-26.at.8.55.19.PM.mov

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
manually test the debug page
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>Copy stylesheet to allow toggling different styles using setStyle without overwriting some of the properties</changelog>

…o used the default value

…eady set for default

test/unit/ui/map.test.js

SnailBones · 2022-05-27T18:55:57Z

src/style/style.js

@@ -309,7 +309,7 @@ class Style extends Evented {
        }

        this._loaded = true;
-        this.stylesheet = json;
+        this.stylesheet = extend({}, json);


Nice fix 👍

Thanks for the review @SnailBones!! all fixes to unit tests pushed! I chatted with @ansis today who recommended doing deep clone instead of shallow clone as nested objects would still be manipulated with extend

test/unit/ui/map.test.js

SnailBones

Nice work Anna, the fix looks good to me!

The tests have some issues (currently they're passing on main) but with a few tweaks I was able to get them working.

ansis · 2022-05-30T13:54:07Z

src/style/style.js

@@ -1497,6 +1497,10 @@ class Style extends Evented {
        } else { // Updating
            const terrain = this.terrain;
            const currSpec = terrain.get();
+
+            // Add in terrain default values if it was not set in style
+            if (!terrainOptions.hasOwnProperty('exaggeration')) terrainOptions.exaggeration = 1;


I think we probably shouldn't have a hardcoded exception here like this. Is this still necessary now that you've switched from a shallow clone to a deep clone?

My understanding from looking at the code is that this line ensures that when changing to a style with custom exaggeration to one without exaggeration defined, the exaggeration is set to one. Otherwise, the exaggeration would remain the same as the previous style as the call to update it would not be made.

Two things to note for that:

It might be more robust to access the default value from the spec, something like:

import styleSpec from '../style-spec/reference/latest.js'; ... styleSpec.terrain['exaggeration'].default

We have a slightly similar code path for fog: https://github.com/mapbox/mapbox-gl-js/blob/main/src/style/fog.js#L82_L87, so maybe this code could be moved here when setting terrain, and loop through all style spec properties, so that adding new terrain specification properties doesn't break this code and these two root properties would have similar logic

Ah thanks @karimnaaji - I tried finding a way to not have to hardcode this value as a last resort. Much appreciated!

It made more sense to put the loop here. In setTerrain it's looping through the new terrain options and only updating the new properties that aren't equal to the old terrain's options, so if the default property+value isn't added to the new terrain options before this, it won't be passed through to terrain.set.

@avpeery Do you think that we should also move it there for fog? If so, maybe there's a way we could consolidate root properties updates and share this code between the two.

@karimnaaji I think that would definitely work, I'll update that shortly! nice find, thanks!

karimnaaji · 2022-05-31T16:03:05Z

src/style/style.js

@@ -309,7 +309,7 @@ class Style extends Evented {
        }

        this._loaded = true;
-        this.stylesheet = json;
+        this.stylesheet = clone(json);


What is the performance cost of cloning on style loads? It might be negligible but it would be good to know the time taken by this new statement as it can have an impact on map loads.

Here is the benchmark run between this branch and main, negligible difference, all tests passed: https://sites.mapbox.com/benchmap-js-results/runs/709, and then against the lastest version 2.8.2: https://sites.mapbox.com/benchmap-js-results/runs/708

karimnaaji

LGTM! Thanks for running the benchmark @avpeery

…errain and fog

…11942) * clone stylesheet instead of manipulating it directly * style should not contain terrain since it is no longer being overwritten * add unit test * add in default values for terrain when updating * Add test for projections * unit test for terrain exaggeration does not need sources * should have two calls to setStyle to fully test toggling * default value does not need to be added if previous terrain style also used the default value * hasOwnProperty is already a boolean * no need to check currSpec because checks for deep equals and 1 is already set for default * fix unit tests * fix style setting in unit test * accidental deletion * switch to deep clone from extend * better way to find default value * use hasOwnProperty instead * convert to false from undefined * create reusable function for setting transition parameters * update _setTransitionParameters * mix up between duraton/delay values * reduce repetitive code to update style spec and transitions between terrain and fog * revert change

* Add fog render test for empty update parameters (#11900) * Add precision qualifiers to globe_raster fragment shader (#11903) * Add filter features within map view with globe release test (#11888) * add filter features within map view with globe release test * Fix incorrect circle bucket up direction (#11904) * Fix for https://github.com/mapbox/mapbox-gl-native-internal/issues/3426 * Update baselines * ScaleControl: add i18n support (#11850) * ScaleControl: add i18n support * hardcode nautical miles as `nm` for all locales * Add minimum/maximum fields for center/parallels (#11912) * Add new render test for globe antialiasing (#11933) * Fix potential flickering on image source updates (#11928) * Fix flickering of image source on reload * Add unit tests * Fix setStyle breaking with globe view (#11913) * set draping for terrain * use map option for terrain draping check * move to one line * wip: add render test * fixed error with render test * demonstrate issue with mercator * fix background id to update in second setStyle call * bring zoom out to show sphere * Updating expected.png * better readability * fix conditional * fix lint Co-authored-by: Aidan H <[email protected]> * Fix tiles not rendering across antimeridian in globe (#11943) * Add video to globe release tests (#11929) * add ozone video on globe to release tests * formatting * formatting * fix lint issues * simplify play/pause button * added atmosphere * Do not render atmosphere when fog isn't supported by the projection (#11947) * Clone stylesheet to allow toggling different styles with `setStyle` (#11942) * clone stylesheet instead of manipulating it directly * style should not contain terrain since it is no longer being overwritten * add unit test * add in default values for terrain when updating * Add test for projections * unit test for terrain exaggeration does not need sources * should have two calls to setStyle to fully test toggling * default value does not need to be added if previous terrain style also used the default value * hasOwnProperty is already a boolean * no need to check currSpec because checks for deep equals and 1 is already set for default * fix unit tests * fix style setting in unit test * accidental deletion * switch to deep clone from extend * better way to find default value * use hasOwnProperty instead * convert to false from undefined * create reusable function for setting transition parameters * update _setTransitionParameters * mix up between duraton/delay values * reduce repetitive code to update style spec and transitions between terrain and fog * revert change * minimize scale changes when panning in globe view (#11951) * adjust scale at low zoom levels in globe view * improve * Adding render test catching missing tiles with minzoom * Allowing lower zoom level tiles toward the poles * Fix lint * Updating render tests to pass * Updating tolerances to fix failing tests * Updating one more tolerance * Fixing query tests * use GLOBE_ZOOM_THRESHOLD_MIN * rename to mercatorScaleRatio * add comment about matching scale * avoid recentering on terrain if rendering globe * update unit tests * lint Co-authored-by: Aidan H <[email protected]> * Do not set the map language to the user's preferred language by default (#11952) * Do not set the map language to the user's preferred language by default * fix unit test * cleanup * cleanup * fix worldview setter Co-authored-by: Mikko Pulkki <[email protected]> Co-authored-by: Anna Peery <[email protected]> Co-authored-by: Stepan Kuzmin <[email protected]> Co-authored-by: Tristen Brown <[email protected]> Co-authored-by: Aidan H <[email protected]> Co-authored-by: Ansis Brammanis <[email protected]>

clone stylesheet instead of manipulating it directly

27d732b

avpeery added the bug 🐞 label May 27, 2022

avpeery added 3 commits May 26, 2022 18:12

style should not contain terrain since it is no longer being overwritten

462a151

add unit test

29a6a5c

add in default values for terrain when updating

6329e6b

avpeery marked this pull request as ready for review May 27, 2022 04:00

avpeery requested review from ansis, karimnaaji and SnailBones May 27, 2022 04:00

Add test for projections

e0bbf17

avpeery mentioned this pull request May 27, 2022

Skip redundant call to map._updateProjection on style change #11940

Closed

5 tasks

avpeery added 5 commits May 26, 2022 21:47

unit test for terrain exaggeration does not need sources

b2891ae

should have two calls to setStyle to fully test toggling

3e1ec8c

default value does not need to be added if previous terrain style als…

a137d72

…o used the default value

hasOwnProperty is already a boolean

6136c21

no need to check currSpec because checks for deep equals and 1 is alr…

4edd616

…eady set for default

avpeery mentioned this pull request May 27, 2022

Calling setStyle doesn't clear previously set root properties. #11939

Closed

avpeery linked an issue May 27, 2022 that may be closed by this pull request

Toggling different projections with setStyle overwrite projection in style #11916

Closed