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.

Sign up for GitHub

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

Jump to bottom

[MAPS3D-425] introduce skirts for globe rendering #12523

Merged
merged 1 commit into from
Jan 23, 2023
Merged

[MAPS3D-425] introduce skirts for globe rendering #12523

merged 1 commit into from
Jan 23, 2023

Conversation

alexey-romanov
Copy link
Contributor

@alexey-romanov alexey-romanov commented Jan 17, 2023 •
edited
Loading

Absent skirts lead to holes for hight exaggerated terrain in globe mode.

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>introduce skirts for terrain globe mode</changelog>

@alexey-romanov alexey-romanov requested a review from a team as a code owner January 17, 2023 12:43
@alexey-romanov
Copy link
Contributor Author

alexey-romanov commented Jan 17, 2023 •
edited
Loading

Introduce strips for terrain globe rendering. Absent strips lead to holes for high exaggerations.

Before:
before_no_strips
After:
after_with_strips

@alexey-romanov alexey-romanov added bug 🐞 GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform labels Jan 17, 2023
@alexey-romanov
Copy link
Contributor Author

Case with transparent textures not handled -> update with moving skirts to be rendered last will be made

mourner
mourner reviewed Jan 17, 2023
View reviewed changes
src/geo/projection/globe_util.js Outdated
// gridIndices: TriangleIndexArray,
// gridSegments: Array<SegmentVector>
// };
_fillGridMeshWithLods(longitudinalCellsCount: number, latitudinalLods: number[], hasStrip: boolean): GridWithLods {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: should we eliminate the hasStrip flag since it's always true? (I guess this a debugging leftover?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have removed the flag from the interface, but kept it internal for potential debug uses

src/geo/projection/globe_util.js Outdated Show resolved Hide resolved
@alexey-romanov alexey-romanov force-pushed the ar5/terrain_globe_strips branch 3 times, most recently from f3694e9 to 7c8e08d Compare January 18, 2023 09:39
@alexey-romanov alexey-romanov changed the title [MAPS3D-425] introduce strips for globe rendering [MAPS3D-425] introduce skirts for globe rendering Jan 18, 2023
@alexey-romanov alexey-romanov force-pushed the ar5/terrain_globe_strips branch 5 times, most recently from 7a6ac35 to 3c97bdd Compare January 19, 2023 06:17
mpulkki-mapbox
mpulkki-mapbox reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox left a comment

Choose a reason for hiding this comment

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

I noticed some areas where the skirt might be missing.

pole_skirts_missing.mov

src/terrain/draw_terrain_raster.js Outdated
const skirtHeightValue = Math.min(skirtHeight(tr.zoom) * terrain.exaggeration(), 20000);

Copy link
Contributor

Choose a reason for hiding this comment

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

How was the value "20000" chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heuristically, without the limiting factor on certain zoom levels, e.g. 1.5 +- there can be seen triangles penetrating the earth surface. It was revealed by the globe anti-aliasing test with extreme exaggerations

Copy link
Contributor

Choose a reason for hiding this comment

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

on the opposite side of the globe? What are the implications of triangles penetrating the surface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

src/terrain/draw_terrain_raster.js Outdated Show resolved Hide resolved
src/geo/projection/globe_util.js Outdated
indices.emplaceBack(idx + offsetToNextRow, idx + offsetToNextRow + 1, idx + 1);
}
}
segments.push(SegmentVector.simpleSegment(0, indexOffset, vertices.length, indices.length - indexOffset));
Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Are you including skirt triangles into the same segment as the main grid triangles? We could have two different segments: one with skirts and one without skirts. This way we can skip rendering of skirts if the terrain is not enabled (or exaggeration is 0) in globe view.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understood correctly this change will always render skirts?

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to construct index buffer so that skirt triangles are immediately after terrain triangles? This wouldn't increase draw count as the only difference would be the number of rendered indices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently, always, but yeah two segments (one with skirt and one without) can be used depending on terrain exaggeration. I'll do that modification

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/shaders/_prelude_terrain.vertex.glsl Show resolved Hide resolved
src/terrain/draw_terrain_raster.js Show resolved Hide resolved
@alexey-romanov alexey-romanov force-pushed the ar5/terrain_globe_strips branch 4 times, most recently from 5e568a5 to e785d9f Compare January 20, 2023 10:44
@mpulkki-mapbox
Copy link
Contributor

Thanks for addressing the review comments @alexey-romanov! Code changes looks good to me but unfortunately there are still some scenarios where the skirt geometry is missing. It is also possible to see gaps when zooming in and out rapidly as I wrote previously.

image

I would suggest fixing remaining issues where gaps are visible even when camera is not moving. We can defer rest of the cases to a follow-up PR.

@alexey-romanov
Copy link
Contributor Author

@alexey-romanov
Copy link
Contributor Author

Thanks for addressing the review comments @alexey-romanov! Code changes looks good to me but unfortunately there are still some scenarios where the skirt geometry is missing. It is also possible to see gaps when zooming in and out rapidly as I wrote previously.

image

I would suggest fixing remaining issues where gaps are visible even when camera is not moving. We can defer rest of the cases to a follow-up PR.

I was able to repro it locally. It is related to the case of transition between globe and mercator

@alexey-romanov
[MAPS3D-425] introduce skirts for globe rendering
77757cf 
Absent skirts lead to holes for hight exaggerated terrain in globe mode.
@alexey-romanov alexey-romanov force-pushed the ar5/terrain_globe_strips branch from e785d9f to 77757cf Compare January 21, 2023 20:53
@alexey-romanov
Copy link
Contributor Author

Thanks for addressing the review comments @alexey-romanov! Code changes looks good to me but unfortunately there are still some scenarios where the skirt geometry is missing. It is also possible to see gaps when zooming in and out rapidly as I wrote previously.

image

I would suggest fixing remaining issues where gaps are visible even when camera is not moving. We can defer rest of the cases to a follow-up PR.

Should be fixed now

@alexey-romanov alexey-romanov merged commit 9e8e044 into main Jan 23, 2023
@alexey-romanov alexey-romanov deleted the ar5/terrain_globe_strips branch January 23, 2023 07:23
@kallyas kallyas mentioned this pull request Jul 17, 2024
Merged
@yash509 yash509 mentioned this pull request Sep 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpulkki-mapbox mpulkki-mapbox mpulkki-mapbox approved these changes

@mourner mourner Awaiting requested review from mourner

Assignees
No one assigned
Labels
bug 🐞 GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexey-romanov @mpulkki-mapbox @mourner