Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Attempt constraining Y axis ratio first, then X axis #12611

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

brunoabinader
Copy link
Member

This matches GL JS default constraint for the map boundaries. Parity ensured via newly added {basic,bright,satellite}-v9/z0-wide render tests.

@brunoabinader brunoabinader added GL JS parity For feature parity with Mapbox GL JS Core The cross-platform C++ core, aka mbgl labels Aug 12, 2018
@brunoabinader brunoabinader self-assigned this Aug 12, 2018
@brunoabinader brunoabinader force-pushed the fix-constrain-height-only branch 2 times, most recently from 7bfe2ae to 5a9833d Compare August 13, 2018 07:57
@brunoabinader
Copy link
Member Author

@mourner even though GL JS and Native are now in sync with regards to the transform constraint results, the heatmap produced by GL native is now different - it actually looks more correct as the color ramp is more pronounced - could you please check these render tests results and check -
( https://141939-16286131-gh.circle-artifacts.com/0/render-tests )? My suspicion is that this has shown a bug in GL JS side.

@mourner
Copy link
Member

mourner commented Aug 22, 2018

Not sure what's exactly going on but it might be a bug in GL Native — the heatmap is more intense likely because there are two points (original and duplicate from another tile) on top of each other.

@brunoabinader
Copy link
Member Author

Not sure what's exactly going on but it might be a bug in GL Native — the heatmap is more intense likely because there are two points (original and duplicate from another tile) on top of each other.

I've revisited this and found out that GL JS uses the following logic: Attempt to constrain the Y axis if its scale (height / tile size) is bigger than 1 - if not, attempt constrain the X axis instead.

Updating this new logic in GL Native fixes the render test results. @mourner could you please re-review?

@brunoabinader brunoabinader changed the title [core] Do not constrain on X axis in ConstrainMode::HeightOnly [core] Attempt constraining Y axis ratio first, then X axis Oct 1, 2018
@brunoabinader brunoabinader merged commit 19179e0 into master Oct 2, 2018
@brunoabinader brunoabinader deleted the fix-constrain-height-only branch October 2, 2018 13:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants