Skip to content

Fix icon/text collisionBox translation - #8659

Merged
zmiao merged 8 commits into
masterfrom
fix-icon/text-translation
Aug 28, 2019
Merged

zmiao merged 8 commits into
masterfrom
fix-icon/text-translation

Conversation

@zmiao

@zmiao zmiao commented Aug 20, 2019 •

Copy link
Copy Markdown
Contributor

Fix: js part of mapbox/mapbox-gl-native#13526

The root cause for it is that when rendering the symbols, the translate offset is only applied to icon/text, but the corresponding collisionBox is ignored. Also collisionBox in current SymbolBucket needs to be split for icon and text symbol separately, in case they have different translate parameter.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality

@zmiao

zmiao commented Aug 20, 2019 •

Copy link
Copy Markdown
Contributor Author

This change leads to one failing render test: debug/collision. As the rendering order may be changed for icon and text collisionBox, as they are now separated. Results are depicted as following:
expected:
expected

actual:
actual

diff:
diff

@asheemmamoowala

Copy link
Copy Markdown
Contributor

@zmiao It looks safe to update the fixture for that test.

@ansis ansis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me! One small thing: there are some extra spaces added to some lines that didn't otherwise change. If possible it might be good to not include these changes.

@asheemmamoowala asheemmamoowala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@ansis the whitespace changes pass the linter, so maybe the eslint rules need to be updated to enforce a consistent convention here. That can be handled separately IMO

@zmiao

zmiao commented Aug 28, 2019

Copy link
Copy Markdown
Contributor Author

@ansis Whitespace is moved + changelog is added. Thanks for heads up.

@zmiao
zmiao merged commit 6275d07 into master Aug 28, 2019
@zmiao
zmiao deleted the fix-icon/text-translation branch August 28, 2019 11:21
stepankuzmin pushed a commit to stepankuzmin/mapbox-gl-js that referenced this pull request Sep 9, 2019
* init fix

* fix lint error

* fix indentation and wrong variable

* add line placement translate render test

* revert accidently added blank line

* update comments

* update one test fixture

* Add changelog + remove whitespace
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.

3 participants