Skip to content

Fix collision boxes for text with variable placement #10709

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 27, 2021
Merged

Fix collision boxes for text with variable placement #10709

merged 1 commit into from
May 27, 2021
+100 −47

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented May 26, 2021 •
edited
Loading

With the move to evaluating collision boxes on the main-thread ,we missed one case where symbols with text-variable-anchor was still using the box sizing for text determined at layout time.
This was causing collision boxes to start off correct but then drift at fractional zoom levels.

gl.js.v2.2.mov

This PR fixes it by evaluating the box sizing on the main thread.

Before:

Screen Shot 2021-05-25 at 3 26 39 PM

After:

actual

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>Fix collision boxes for text with variable placement</changelog>

Sorry, something went wrong.

Remove layout time textBoxScale and rely on scale evaluated during pl…

Verified

This commit was signed with the committer’s verified signature.
renovate-bot Mend Renovate
GPG key ID: 81C63AE1C224182B
Verified
Learn about vigilant mode
daa19cf 
…acement
@arindam1993 arindam1993 requested a review from karimnaaji May 26, 2021 01:20
@karimnaaji
Copy link
Contributor

karimnaaji commented May 26, 2021 •
edited
Loading

cc @zmiao for native

@karimnaaji karimnaaji requested a review from zmiao May 26, 2021 22:29
karimnaaji
karimnaaji approved these changes May 26, 2021
View reviewed changes
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch @arindam1993 , looks good to me.

@arindam1993 arindam1993 merged commit 30faf71 into main May 27, 2021
@arindam1993 arindam1993 deleted the fix-variable-offset-collision branch May 27, 2021 17:34
rreusser pushed a commit that referenced this pull request Jun 21, 2021
@rreusser
Fix collision boxes for text with variable placement (#10709)
cd2d343
@rreusser rreusser mentioned this pull request Jun 21, 2021
Merged
rreusser added a commit that referenced this pull request Jun 21, 2021
@rreusser
v2.3.1 additional cherry-picks (#10790)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
c377a90 
* Migrate github size chekcer app and aws creds to mbx-ci (#10719)

* Fix collision boxes for text with variable placement (#10709)

* Add 10709 to changelog

Co-authored-by: Arindam Bose <arindam.bose@mapbox.com>
SnailBones pushed a commit to SnailBones/mapbox-gl-js that referenced this pull request Jul 15, 2021
@SnailBones
Fix collision boxes for text with variable placement (mapbox#10709)
c481665
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karimnaaji karimnaaji

@zmiao zmiao

Assignees
No one assigned
Labels
bug 🐞
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arindam1993 @karimnaaji @zmiao