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

Apply text-keep-upright after text-offset #4779

Merged
merged 3 commits into from
Jun 8, 2017
Merged

Apply text-keep-upright after text-offset #4779

merged 3 commits into from
Jun 8, 2017

Conversation

anandthakker
Copy link
Contributor

Closes #2350

@anandthakker anandthakker force-pushed the 2350 branch 2 times, most recently from 8c55a7e to 6a8919a Compare June 2, 2017 21:41
@anandthakker anandthakker requested a review from ChrisLoer June 2, 2017 21:50
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

I think there's a collision problem here: the CollisionFeature for a label will get its box from the Shaping, which assumes it's upright, but then if the quads are drawn flipped their offsets won't match the offsets of their collision boxes. We re-generate the collision tile every time we do placement, so it's no problem to update the collision boxes, but we may want to move the text-offset logic from CollisionFeature (which is just generated at tile parse time) to CollisionTile#insert/placeCollisionFeature.

@ansis
Copy link
Contributor

ansis commented Jun 5, 2017

I think this change makes sense but are there any cases where the current behavior is the desired one? Is this a breaking change?

@anandthakker
Copy link
Contributor Author

@ChrisLoer looks like the collision problem already exists -- filed as a separate issue here: #4798

@anandthakker
Copy link
Contributor Author

@jfirebaugh got confirmation from @nickidlugash that it's reasonable from the carto team's point of view to consider this a bug fix.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for looking through the details of how this interacts with *-anchor properties, etc

This should be good to merge and I'll rebase #4781 on it

@anandthakker anandthakker merged commit 9e9aede into master Jun 8, 2017
@anandthakker anandthakker deleted the 2350 branch June 8, 2017 17:04
@jfirebaugh
Copy link
Contributor

Do the integration tests pass in -native?

@anandthakker
Copy link
Contributor Author

@jfirebaugh just chatted with @ansis about this when I realized I'd forgotten to mark them ignored -- since the shaders here are already ahead of native, he's going to make sure this fix is included in the stuff he's working on.

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.

4 participants