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

[core] make symbols fade out faster while zooming out #15416

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Aug 19, 2019

Zooming out can make symbols overlap quickly. After zooming out quickly the area previously covered by the viewport has a lot of colliding labels while the surrounding area has no labels. This difference produces an unwanted effect:

Screen Shot 2019-08-19 at 7 51 39 PM

This reduces that effect by:

  • reducing the fade duration while zooming out
  • doing placement more frequently while zooming out

ports https://github.com/mapbox/mapbox-gl-js/pull/8628/files

In -js we're still trying to figure out whether this introduces jankiness. I've only tested this in the glfw app on -native and it seems to work as smoothly as master. Placement is faster on -native so this should be an ok change.

To test, zoom in to a dense area and then zoom out as quickly and suddenly as possible. The main thing to check is that it doesn't introduce extra jank.

  • tested on a device

@langsmith @friedbunny could you help test this on a device?

Zooming outcan make symbols overlap quickly. The area previously covered
by the viewport is covered by a lot of colliding labels while the
surrounding area has no labels. This difference produces an unwanted
effect.

This reduces that effect by:
- reducing the fade duration while zooming out
- doing placement more frequently while zooming out
@chloekraw chloekraw added this to the release-queso milestone Aug 20, 2019
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.

The code changes here look good. I've tested this on the macosapp with a touchpad, but would like to see confirmation of on-device improvements as well before we decide to move forward with this change.

@ansis Is there any way to test these types of changes? Given that they are dependent on system performance, I don't think a render test with a wait will lead to reliable results. And there aren't any existing unit tests for the Placement class.

@chloekraw chloekraw added the Core The cross-platform C++ core, aka mbgl label Aug 20, 2019
@pozdnyakov
Copy link
Contributor

The code looks great and the behavior is much better now, however I can still see the frames, where labels are grouping:
text_grouping

There are no such artifacts, if DEFAULT_TRANSITION_DURATION is set to zero - would be nice to mimic this behavior on quick zoom out.

And a linear adjustment seems to make more sense.
@ansis
Copy link
Contributor Author

ansis commented Aug 20, 2019

There are no such artifacts, if DEFAULT_TRANSITION_DURATION is set to zero - would be nice to mimic this behavior on quick zoom out.

@pozdnyakov I think b509547 should help with this. The / 1.5 can be tweaked to make this more or less sensitive. 1.5 seemed to hide things fast enough while zooming quickly without speeding up fading too much for normal interactions. It also switches to a linear adjustment which should actually be more suitable.

It would be great if someone could test on a device. Other than that, feel free to merge.

@alexshalamov
Copy link
Contributor

alexshalamov commented Aug 20, 2019

@ansis I will check how it looks on Pixel2.


Looks fine on device, pinch zoom-out is not as fast as scroll (trackpad / wheel) on desktop, thus, label clustering is almost invisible.

@pozdnyakov
Copy link
Contributor

@ansis looks better now, thanks!

@pozdnyakov pozdnyakov merged commit 888d2b0 into master Aug 20, 2019
@pozdnyakov pozdnyakov deleted the improve-symbol-fade-zoom-out branch August 20, 2019 10:34
@friedbunny friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 20, 2019
@chloekraw
Copy link
Contributor

@ansis could you add a changelog entry in a new pr?

@mapbox/maps-android @mapbox/maps-ios noting that we should definitely spend time testing this change on low-end devices with the beta goes out.

@tobrun tobrun removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Aug 22, 2019
@tobrun
Copy link
Member

tobrun commented Aug 22, 2019

removing changelog label, this was added in #15443

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

Successfully merging this pull request may close these issues.

None yet

7 participants