Skip to content

support icon-text-fit with vertical text - #8835

Merged
ansis merged 7 commits into
masterfrom
vertical-icon-text-fit
Oct 15, 2019
Merged

ansis merged 7 commits into
masterfrom
vertical-icon-text-fit

Conversation

@ansis

@ansis ansis commented Oct 4, 2019

Copy link
Copy Markdown
Contributor

This ports support for icon-text-fit combined with vertical text. This works with both variable and non-variable placement. fix #8806

A second vertical icon is added to the buffers along with a second vertical icon collision box. Then at placement time it selects the correct icon depending on which version of text it ended up placing.

Vertical text used to be toggled by updating the vertex buffers every frame. I switched this so that it just uses the opacity that only gets updated whenever placement is done. This is simpler and faster.

This enables

  • render-tests/text-writing-mode/point_label/cjk-variable-anchors-vertical-horizontal-mode-icon-text-fit
  • render-tests/text-writing-mode/point_label/mixed-multiline-vertical-horizontal-mode-icon-text-fit

Launch Checklist

  • briefly describe the changes in this PR
  • post benchmark scores
  • manually test the debug page

@ansis
ansis requested review from arindam1993 and vakila October 4, 2019 20:14
@asheemmamoowala asheemmamoowala added this to the release-sangria milestone Oct 7, 2019

@arindam1993 arindam1993 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.

LGTM except for some code duplication.

Comment thread src/symbol/placement.js Outdated
Comment thread src/symbol/placement.js
const placedOrientation = this.placedOrientations[symbolInstance.crossTileID];
const verticalHidden = (placedOrientation === WritingMode.horizontal || placedOrientation === WritingMode.horizontalOnly) ? 1 : 0;
const horizontalHidden = placedOrientation === WritingMode.vertical ? 1 : 0;

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.

Do we need an assertion here that both verticalHidden and horizontalHidden are not 0.
horizontallHidden can also be !verticalHidden, right? since we have 3 writing modes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That made sense to me but I tried this and it turns out they can both be 0. Switching between vertical and horizontal glyphs happens in updateLineLabels(...) in symbol/projection.js and both versions need to be not hidden for that to work. It relies on a placedOrientation not being set for line labels. This is pretty tricky and it would be great to try to clean this up but it's out of the scope of this pr

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.

Ah ok!, could you leave a comment about this?

Comment thread src/symbol/placement.js Outdated
Comment thread src/symbol/placement.js Outdated
Comment thread src/symbol/placement.js Outdated
@ansis
ansis merged commit 79c3534 into master Oct 15, 2019
@ansis
ansis deleted the vertical-icon-text-fit branch October 15, 2019 20:49
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.

port support for using icon-text-fit with vertical text

3 participants