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

sort icons and text together so icons can overlap text #10123

Closed
wants to merge 1 commit into from

Conversation

xabbu42
Copy link

@xabbu42 xabbu42 commented Nov 20, 2020

This pull request attempts to fix issue #10002 by sorting the icon and the text part of a symbol together
instead of one after the other. I'm not sure if this is just a bug fix or a change in the style spec. I'm also not
sure if this needs a port to mapbox-gl-native.

Keep in mind when reviewing this pull request that beside of javascript itself I'm not at all familiar
with any of the tech used (i.e. nodejs, webgl, flow etc).

@mapbox/map-design-team @mapbox/static-apis

  • include before/after visuals or gifs if this PR includes visual changes
  • post benchmark scores
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@xabbu42
Copy link
Author

xabbu42 commented Nov 30, 2020

Screenshot 2020-11-30 at 18 42 59
Screenshot 2020-11-30 at 18 43 29

This two screenshots show the visual difference for two overlapping symbols. The first is from main and the text of both symbols are visible despite the overlap. The second shows the fix where the icon of the overlapping symbol is drawn over the text.

@xabbu42
Copy link
Author

xabbu42 commented Dec 1, 2020

Even with a fresh checkout of mapbox-gl-js main I get an error for the SymbolLayout benchmark: TypeError: i.tileID is undefined.

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2020

CLA assistant check
All committers have signed the CLA.

@xabbu42
Copy link
Author

xabbu42 commented Jan 28, 2021

possibly also fixes #4064

@shayanaijaz
Copy link

Any updates on this issue or a potential merge?

@ansis
Copy link
Contributor

ansis commented Mar 10, 2022

I apologize for the silence here.

Thanks for opening this! #10002 is a messy bug to fix. Unfortunately the way this is implemented, it adds more performance overhead and would make some styles significantly slower. Right now we need to prioritize not regressing the performance of those styles over the ideal rendering. This fixes the rendering, and it's unfortunate that we can't merge it.

Since we're not able to move a complete fix forward right now, I'm going to close this. I apologize for not making this call earlier. I'll reopen if we pick this up again.

Thanks!

@ansis ansis closed this Mar 10, 2022
@enersis-pst
Copy link
Contributor

Is there a chance to fix this issue?
There are so many people having this problem. Would be really nice to see this fixed.

#10021
#4709
https://stackoverflow.com/questions/51245621/avoid-text-clipping-of-symbols-in-mapbox-gl-js
#4717
....

@entioentio
Copy link

entioentio commented Mar 30, 2023

Since we're not able to move a complete fix forward right now, I'm going to close this. I apologize for not making this call earlier. I'll reopen if we pick this up again.

@ansis it seems that enough people are runnning into this limitation over the years. Wouldn't it be handy to have it explicitly mentioned somewhere in the docs?

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

Successfully merging this pull request may close these issues.

6 participants