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

Fix layers rendering after fill-extrusion #15065

Merged
merged 2 commits into from
Jul 9, 2019
Merged

Fix layers rendering after fill-extrusion #15065

merged 2 commits into from
Jul 9, 2019

Conversation

astojilj
Copy link
Contributor

@astojilj astojilj commented Jul 7, 2019

This fixes following issues:

  • Fix some false passing combinations/fill-extrusion-translucent--XXXX tests.
  • Fix and enable other, failing but ignored, combinations/fill-extrusion-translucent--XXXX and combinations/XXXX--fill-extrusion tests.
  • Fix rendering of layers that are on top of fill-extrusion layers.

state.getProjMatrix(nearClippedProjMatrix, 100) caused that tests with size 64x64 were not
rendering fill extrusions: far plane calculated as 96.9 and near plane set to 100 was the cause.

Near plane is changed from hardcoded 100 to depend on state.getCameraToCenterDistance() - producing similar value but one that follows max zoom.

This caused that e.g. combinations/fill-extrusion-translucent--fill-opaque was falsely passing as only fill-opaque layer got rendered.

combinations/fill-extrusion-translucent--XXXX tests expose regression #14844 (comment) in #14844, #14779.

Fix (opaquePassCutoff, is3D) is ported from mapbox/mapbox-gl-js#7821

Fixes: #14844, #14779, #15039

@astojilj astojilj self-assigned this Jul 7, 2019
@astojilj astojilj added GL JS parity For feature parity with Mapbox GL JS high priority labels Jul 7, 2019
@astojilj astojilj requested a review from ansis July 7, 2019 11:38
pozdnyakov
pozdnyakov reviewed Jul 8, 2019
View reviewed changes
src/mbgl/renderer/render_layer.hpp Show resolved Hide resolved
src/mbgl/renderer/render_orchestrator.cpp Outdated Show resolved Hide resolved
platform/node/test/ignores.json
"render-tests/combinations/fill-translucent--fill-extrusion-translucent": "needs investigation",
"render-tests/combinations/line-translucent--fill-extrusion-translucent": "needs investigation",
"render-tests/combinations/raster-translucent--fill-extrusion-translucent": "needs investigation",
"render-tests/combinations/symbol-translucent--fill-extrusion-translucent": "needs investigation",
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@tmpsantos tmpsantos mentioned this pull request Jul 8, 2019
Closed
@friedbunny friedbunny added this to the release-picklejuice milestone Jul 8, 2019
@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl needs changelog Indicates PR needs a changelog entry prior to merging. rendering labels Jul 8, 2019
@astojilj astojilj added needs backport Indicates PR needs to be cherrypicked into a previous release branch. regression labels Jul 9, 2019
@astojilj
Copy link
Contributor Author

astojilj commented Jul 9, 2019

"needs backport" added because of this comment:

@mapbox/maps-ios @mapbox/maps-android, we should also release patches of iOS 5.0, 5.1 and Android 8.0, 8.1 when this is fixed.

@tobrun
Copy link
Member

tobrun commented Jul 9, 2019

"needs backport" added because of this comment:

@mapbox/maps-ios @mapbox/maps-android, we should also release patches of iOS 5.0, 5.1 and Android 8.0, 8.1 when this is fixed.

Concretely this means cherry picking changes to release-nectar and release-oolong branches

pozdnyakov
pozdnyakov approved these changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

LGTM % optional nit

src/mbgl/renderer/render_layer.hpp Show resolved Hide resolved
src/mbgl/renderer/render_orchestrator.cpp Outdated Show resolved Hide resolved
astojilj added 2 commits July 9, 2019 14:12
@astojilj
Fix layers rendering after fill-extrusion
c852c04 
This fixes following issues:
* Fix some false passing combinations/fill-extrusion-translucent--XXXX tests
* Fix and enable other, failing but ignored, combinations/fill-extrusion-translucent--XXXX tests
* Fix rendering of layers that are on top of fill-extrusion layers

state.getProjMatrix(nearClippedProjMatrix, 100) caused that tests with size 64x64 were not
rendering fill extrusions: far plane calculated as 96.9 and near plane set to 100 was the cause.
near plane is changed from hardcoded 100 to depend on state.getCameraToCenterDistance() - producing
similar value but one that follows max zoom.

This caused that e.g. combinations/fill-extrusion-translucent--fill-opaque was falsely passing as
only fill-opaque layer got rendered.

combinations/fill-extrusion-translucent--XXXX tests expose regression #14844 (comment) in #14844, #14779.

Fix (opaquePassCutoff, is3D) is ported from mapbox/mapbox-gl-js#7821

Fixes: #14844, #14779, #15039
@astojilj
Issue #15065 changelogs and nit fix.
3ffc14a
@astojilj astojilj force-pushed the astojilj-opaquePassCutOff branch from a89473c to 3ffc14a Compare July 9, 2019 11:28
@astojilj astojilj requested a review from 1ec5 as a code owner July 9, 2019 11:28
@astojilj astojilj requested a review from a team July 9, 2019 11:28
@astojilj astojilj merged commit 286bc66 into master Jul 9, 2019
@tmpsantos tmpsantos deleted the astojilj-opaquePassCutOff branch July 9, 2019 15:17
@tmpsantos tmpsantos mentioned this pull request Jul 9, 2019
Closed
@chloekraw chloekraw added the bug label Jul 10, 2019
@friedbunny friedbunny removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jul 14, 2019
@chloekraw chloekraw mentioned this pull request Jul 18, 2019
Merged
@LukasPaczos LukasPaczos mentioned this pull request Jul 18, 2019
Closed
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
[core][cp] Backport #15065 to nectar
aa23964
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
Fix wrong #15065 nit fix
7cb86a2 
Change in 3ffc14a
was a typo while addresssing review nit: while it didn't affect render tests it doesn't work properly in all cases.

Fixes: #14844, #14779, #15039
@astojilj astojilj mentioned this pull request Jul 19, 2019
Merged
astojilj added a commit to mapbox/mapbox-gl-js that referenced this pull request Jul 19, 2019
@astojilj
Render test with multiple fill extrusions and circle layers interleaved
602fd7d 
Our combinations fill-extrusion render tests mostly combine fill extrusion with one other layer.
This just serves for verifying it using more layers.
It doesn't expose any particular issue - serves for verifying mapbox/mapbox-gl-native#15065
@astojilj astojilj mentioned this pull request Jul 19, 2019
Merged
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
Fix wrong #15065 nit fix
d7f28c3 
Change in 3ffc14a
was a typo while addresssing review nit: while it didn't affect render tests it doesn't work properly in all cases.

Fixes: #14844, #14779, #15039
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
[core][cp] Backport #15065 to nectar
93adb81
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
Fix wrong #15065 nit fix
1ae7ff4 
Change in 3ffc14a
was a typo while addresssing review nit: while it didn't affect render tests it doesn't work properly in all cases.

Fixes: #14844, #14779, #15039
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
[core][cp] Backport #15065 to oolong
97cd09c
astojilj added a commit that referenced this pull request Jul 19, 2019
@astojilj
[core][cp] Backport #15065 to oolong
b791a85
astojilj added a commit to mapbox/mapbox-gl-js that referenced this pull request Jul 19, 2019
@astojilj
Render test with multiple fill extrusions and circle layers interleaved
9a4254e 
Our combinations fill-extrusion render tests mostly combine fill extrusion with one other layer.
This just serves for verifying it using more layers.
It doesn't expose any particular issue - serves for verifying mapbox/mapbox-gl-native#15065
@astojilj astojilj mentioned this pull request Jul 19, 2019
Merged
@LukasPaczos LukasPaczos mentioned this pull request Jul 22, 2019
Closed
astojilj added a commit that referenced this pull request Jul 23, 2019
@astojilj
Fix wrong #15065 nit fix
cefec34 
Change in 3ffc14a
was a typo while addresssing review nit: while it didn't affect render tests it doesn't work properly in all cases.

Fixes: #14844, #14779, #15039
@langsmith langsmith mentioned this pull request Jul 31, 2019
Merged
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexshalamov alexshalamov alexshalamov approved these changes

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@tmpsantos tmpsantos tmpsantos approved these changes

@ansis ansis Awaiting requested review from ansis

@1ec5 1ec5 Awaiting requested review from 1ec5

Assignees

@astojilj astojilj

Labels
bug Core The cross-platform C++ core, aka mbgl GL JS parity For feature parity with Mapbox GL JS high priority needs backport Indicates PR needs to be cherrypicked into a previous release branch. regression rendering
Projects
None yet
Milestone
release-picklejuice
Development

Successfully merging this pull request may close these issues.

BC break: circle layer is drawn in different z-index
7 participants
@astojilj @tobrun @tmpsantos @pozdnyakov @alexshalamov @friedbunny @chloekraw