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

symbol layer fades in upon setData #5716

Closed
andrewharvey opened this issue Nov 20, 2017 · 32 comments · Fixed by #5741
Closed

symbol layer fades in upon setData #5716

andrewharvey opened this issue Nov 20, 2017 · 32 comments · Fixed by #5741
Labels

Comments

@andrewharvey
Copy link
Collaborator

andrewharvey commented Nov 20, 2017

mapbox-gl-js version: git bisect reveals this is a regression in 1355618 (#5150)

When updating a source with setData any symbol layers using that source will fadein again after the setData. This causes the symbol to flicker when setData is fired frequently such as from a cursor event.

See this in action at https://jsbin.com/kezucog/edit?html,output (just move the cursor around the screen). This wasn't a problem in 0.41.0, but is a problem with 0.42.0 and I pinpointed it to the commit mentioned.

I can see that GL JS now does cross source symbol placement so previously the "Mapbox DC" (from the example) would overlap existing labels, now it will actually fade out the other labels to make room for it. To get the 0.41 behaviour back it's just a matter of using "text-ignore-placement": true, "icon-ignore-placement": true in that layer's layout properties. However even with this, the label still fades in again every time it is updated.

/cc @ChrisLoer

@ryanbaumann
Copy link
Contributor

ryanbaumann commented Nov 20, 2017

I can replicate this issue as described by @andrewharvey on Chrome, Firefox, and Edge. Examples:

  1. 0.41.0 example - setData() on a symbol layer animates symbols on setData() without fade.
  2. 0.42.0 example - setData() on a symbol layer causes significant icon-image fade.

@ChrisLoer
Copy link
Contributor

@ansis This sounds like it's probably another issue with how CrossTileSymbolIndex updates when a tile reload happens (like #5491, except this time it's from setData instead of setFilter)? I'm not sure why the fix for #5491 isn't also working here, but this seems like something that the back-port from native would address. Do you see a quick fix we can do now for this, or is the back-port coming so soon it doesn't matter?

@ryanbaumann I shouldn't make light of broken functionality, but that fleet of ghost-planes crossing the South Pacific is... kind of awesome.

@lbud
Copy link
Contributor

lbud commented Nov 20, 2017

Also reported as a bug with the "Animate a point along a route" example in #5490 (comment).

@pathmapper
Copy link
Contributor

pathmapper commented Nov 21, 2017

I think it is the same issue, so posting it here:

There is also an effect on a fill-pattern:

image

Here are two examples, just zoom in to see the issue:

v.0.41.0 example -> the blurry lines are only a short moment visible when zooming

v.0.42.2 example -> the blurry lines are staying visible at certain zoom levels

@ansis
Copy link
Contributor

ansis commented Nov 22, 2017

@pathmapper I think the underlying cause for that is different so I split your comment into a separate issue: #5740 (comment)

@pathmapper
Copy link
Contributor

@ansis sure and thanks!

@livemixlove
Copy link

I've upgraded to v0.43.0-beta.1, and while the symbol flickering seems improved, I'm still seeing it happen:

fading-icons

@ChrisLoer
Copy link
Contributor

@livemixlove I'm not sure how you're adding that marker to the map, but this looks like expected collision detection behavior to me. Starting with v.42, we detect label collisions between symbols from different sources (previously, it was a limitation of GL JS that we couldn't detect them).

If you want the symbols to be allowed to overlap, you have to explicitly use the *-ignore-placement and/or *-allow-overlap options in your style.

@livemixlove
Copy link

The label collision feature is awesome. But, notice how the marker disappears (quickly fades in and out ) in a place where there should be no collision (below "Northern BLVD"). You might have to watch the gif a few times. This looks like a bug to me, unless I need to be setting some new parameters for layers.

@livemixlove
Copy link

Also, when I say "marker", I'm only referring to the green marker being dragged.

@livemixlove
Copy link

I added

"icon-ignore-placement": true,
"icon-allow-overlap": true,

to the green icon's layout properties, and I still see the same behavior.

@ChrisLoer
Copy link
Contributor

Ah OK thanks for the clarification, I was paying attention to the wrong thing -- looking at the labels underneath instead of that little flicker in the marker itself.

A guess: this might be happening when the marker crosses a tile boundary (you can see tile boundaries by setting map.showTileBoundaries = true)? Crossing from one tile to the next would skip the justReloaded check we put in to try to support "setData animation without fading". This might be an argument for adding per-layer collision fade duration configurations, so that we could just disable the fading for layers animated with setData....

@livemixlove
Copy link

More info:

  • I turned on showTileBoundaries and was able to reproduce the effect without crossing a boundary.

  • I just realized that there's actually a second marker beneath the green marker. So I'm actually dragging two markers (one larger visible one and one smaller obscured) in the gif. Both markers have "icon-allow-overlap": true.

icons-fading2

@ChrisLoer
Copy link
Contributor

Shucks, I thought it was such a good guess.

🤔 Do you think you could put together a minimal JSFiddle demonstrating the behavior?

@livemixlove
Copy link

livemixlove commented Dec 15, 2017

Ok, I made a minimal example. I modified the https://www.mapbox.com/mapbox-gl-js/example/drag-a-point/ example:

https://codepen.io/anon/pen/xpwqpe (make sure to add your accessToken)

If you increase the number of geojson features, the effect becomes more pronounced. On line 65 I add 2000 additional features that aren't being dragged.

I think the bug happens when you use "type": "symbol" for the layer. The glitch isn't visible to me when you use circle

I'm on OSX 10.12.3 , Chrome Version 62.0.3202.94 (Official Build) (64-bit)

@andrewharvey
Copy link
Collaborator Author

I can replicate the flashing, to make it easier to see I forked your example to https://codepen.io/anon/pen/JMYWBg adding:

  • larger icon-size
  • icon-ignore-placement: true to allow other symbols to be shown even if they overlap the icon (eg labels)

@livemixlove
Copy link

@ChrisLoer , should we reopen this issue?

@ChrisLoer ChrisLoer reopened this Dec 18, 2017
@ChrisLoer
Copy link
Contributor

Yeah, let's re-open for now. I think we're probably looking at a different underlying problem, but might as well track it here until we figure it out.

Thanks for the codepen!

@livemixlove
Copy link

Related note: I'm getting buggy mouseover/queryRenderedFeatures behavior. I think it's the same issue where passing large objects via setData is making the layer disappear briefly.

v0.37.0
hover-normal

v0.43.0-beta.1
glitchy-hover

@ChrisLoer
Copy link
Contributor

That's a good hint @livemixlove, and there's definitely a problem in queryRenderedFeatures. I filed it as #5887.

I'm not certain it explains what you're seeing (depends on a lot of timing), but it definitely could.

@ChrisLoer
Copy link
Contributor

@andrewharvey @livemixlove I'm not actually sure what I'm looking for in your codepens. I set the map's fadeDuration to 2000ms so I could definitely catch any spurious fading-in, and I don't see any when I move the circle around. The performance is generally bad, though, as long as the point with 2000 features is within the set of loaded tiles: calling setData this frequently just really stresses out our tile loading infrastructure. I have ideas about how to improve that, but first I want to track down whatever was happening at #5716 (comment).

@andrewharvey
Copy link
Collaborator Author

@ChrisLoer In https://codepen.io/anon/pen/JMYWBg when you drag that circle around (you need to hover over the centre to get the grab mouse handler, and then you can drag it), and move it quickly it will flash in and out, rather than just moving from one location to the next as you'd expect. I'm not sure if this is the intended behaviour or a bug, ie. should it really fade in then out when using set data? When animating like this you probably want to skip the fade in/out. I just made a few changes to @livemixlove example to try to demonstrate the flashing easier.

@livemixlove
Copy link

This is what it looks like to me.

codepen-glitch

Does fadeDuration force the full duration to happen or does it just slow the effect? I can't find any documentation. Might setting fadeDuration to longer make the fade glitch less obvious?

@livemixlove
Copy link

I still see the effect with new mapboxgl.Map({...,fadeDuration: 2000,...}). It just the page/flashing is a little different.

@ChrisLoer
Copy link
Contributor

Ah! I definitely do not see that on my machine. Not sure why.

You're right, fadeDuration is undocumented. It defaults to 300ms. In this case, setting it to a longer value will make the glitch more obvious because the circle will disappear instantly and then fade in over the longer period. But it probably doesn't matter -- 300ms is enough to see the effect, I'm just apparently getting different results from you two...

@livemixlove
Copy link

livemixlove commented Dec 19, 2017

My machine: OSX 10.12.3 , Chrome Version 62.0.3202.94
MacBook Pro (Retina, 13-inch, Early 2015)

@ChrisLoer
Copy link
Contributor

I'm on macOS 10.13.2, but I doubt it makes a difference.

In the example you showed, it's crossing over tile boundaries (or at least I think it is because it's at null island, and tiles always come together there) -- can you turn tile boundaries on and reproduce in an area without them? Because this can definitely happen crossing tile boundaries.

The tile boundary issue is just a limitation of how we do fading (we don't have any reliable way to tell that two icons in two different tiles are conceptually "the same" and thus shouldn't be cross-faded): I think that's an argument for allowing individual layers to opt-out of fade animations.

@livemixlove
Copy link

Yep, same as before without crossing tiles:
tiles-fade

(the lack of cursor motion is just because the example lags so much behind the mouse movement)

@ChrisLoer
Copy link
Contributor

Here's the closest I can get: one missing circle on frame 20. But this is not the same as what you're seeing -- note that there's no fading.

set_data_flicker

@ansis is almost ready to land a PR that will change this code pretty significantly: #5890. Might be worth checking out that branch to see if you can reproduce with that code as well.

@livemixlove
Copy link

@ChrisLoer Maybe your computer is just much faster? I'm not sure if Chrome's dev tools CPU throttling would help recreate.

@ansis
Copy link
Contributor

ansis commented Dec 19, 2017

I can reproduce both issues on #5890. I'll try to find the cause of the flickering within the tile bounds.

@ansis
Copy link
Contributor

ansis commented Jan 9, 2018

@andrewharvey @livemixlove thanks for the test cases. They should be fixed by #5890 which was just merged into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants