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

SymbolLayer dissapears #10559

Closed
tobrun opened this issue Nov 24, 2017 · 7 comments
Closed

SymbolLayer dissapears #10559

tobrun opened this issue Nov 24, 2017 · 7 comments
Labels
Core The cross-platform C++ core, aka mbgl

Comments

@tobrun
Copy link
Member

tobrun commented Nov 24, 2017

When adding a 1-point backed SymbolLayer to the map and assigning it an iconImage,
I'm seeing the following behaviour on master (not on release branch):

ezgif com-video-to-gif 1

Code

cc @ChrisLoer @ansis

@tobrun tobrun added bug Core The cross-platform C++ core, aka mbgl labels Nov 24, 2017
@ChrisLoer
Copy link
Contributor

I may be missing something, but isn't this what we expect now that we've implemented cross-source symbol placement? Before, this icon was placed with its own source so it could never collide with anything. Now, it's placed along with everything else, so if you want to avoid collision you need to use icon-allow-overlap/icon-ignore-placement.

I wonder how many users unintentionally depend on the current lack of cross-source placement. We could build a "grandfathered" mode in that kept the old behavior by default, but in most cases I imagine the new behavior is closer to what users would expect, and if the new behavior causes a problem it should be easy to adjust your style to get the behavior you want (and a style that's cross-source aware could still degrade to the same behavior with earlier versions of the SDK that don't support cross-source placement).

@tobrun tobrun removed the bug label Nov 27, 2017
@tobrun
Copy link
Member Author

tobrun commented Nov 27, 2017

Thank you for adding context, I had a hunch that this was expected behavior but didn't knew about icon-ignore-placement to get it working again. With that in place it's the same as before!

I don't mind this becoming default behavior though this would mean that this feature is a Semver change. Re. grandfathering users, in the past we have written migration docs for major version updates, this change is something that could be included in that.

cc @mapbox/ios

@1ec5
Copy link
Contributor

1ec5 commented Nov 28, 2017

Unless annotations suddenly start disappearing due to collision, I think it’s fine to ship this change without worrying about backwards incompatibility. For the iOS SDK, this change is going into a semver-major release anyways. 😉

@tobrun
Copy link
Member Author

tobrun commented Dec 21, 2017

I have been experimenting with animating symbol layer items,
when working on the release branch everything works:

ezgif com-video-to-gif 19

But the same code is resulting in the following on master:

ezgif com-video-to-gif 21

What the code is doing is use an Android SDK animator to perform the animation, which each animation callback we are calling setGeoJSON on the GeoJSON source. Is this something we can optimise symbol placement for?

@ChrisLoer
Copy link
Contributor

ChrisLoer commented Dec 21, 2017

Is this something we can optimise symbol placement for?

Yeah, we're in the middle of realizing we broke this on GL-JS. We think we have fixes in place that we can port to address this: mapbox/mapbox-gl-js#5716 (native port is #10536), mapbox/mapbox-gl-js#5837 (hopefully doesn't exist on native), mapbox/mapbox-gl-js#5887 (#10778)

/cc @ansis

@ChrisLoer
Copy link
Contributor

This should be mostly fixed by #10899 when it goes in. #10778 is still an issue but probably not relevant here.

@tobrun
Copy link
Member Author

tobrun commented Jan 11, 2018

@ChrisLoer thank you for following up, was able to confirm that the issue is resolved with #10899:

ezgif com-video-to-gif 28

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

No branches or pull requests

4 participants