-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Account for negative terrain elevation in frustum far plane definition #10432
Conversation
31ced0c
to
ae53b54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I poked around and had come up with a slightly different permutation of the same thing. I just had one little nit about a redundant abs.
It's definitively better so I marked it "approved", but I checked out this branch and did notice one glitch. When I initially load, it sometimes does or does not trigger a final redraw which applies this clipping. The result is sometimes-bad clipping which does not resolve itself until an externally triggered redraw. I'm not great with async loading behavior yet, but is there a way to trigger a redraw when data has loaded and the minimum has been computed?
src/terrain/terrain.js
Outdated
@@ -740,6 +741,15 @@ export class Terrain extends Elevation { | |||
return {efficiency: (1.0 - uncacheableLayerCount / drapedLayerCount) * 100.0, firstUndrapedLayer}; | |||
} | |||
|
|||
getMinElevationBelowMSL(): number { | |||
let min = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. I think this is a better initialization than +INFINITY, since when things were initially loading, I found that infinity percolated through to the camera adjustment.
Oh, sorry, the location of my screenshot: http://localhost:9966/debug/terrain-debug.html#12.38/31.37145/35.37661/0/9 |
Co-authored-by: Ricky Reusser <[email protected]>
Co-authored-by: Ricky Reusser <[email protected]>
That's a good catch, I will debug more to see how it can be directly accounted for without redraw.
I wouldn't be surprised if there are other locations in our code where we make an assumption that all values are >= 0, especially in camera related behavior. Do you remember the location of the gif where this was triggered? |
Anywhere around the Dead Sea works pretty well 😄 E.g. http://localhost:9966/debug/terrain-debug.html#17.1/31.417303/35.379056/0/9 (A somewhat niche issue, at least until someone imports bathymetry data 🐳 ) |
I took some time to look at the issue mentioned around the frustum plane not being pushed enough until an interaction on first load. It's a bit of a hard one to reproduce, but so far what I noticed is that we're only off by one frame. I'm a bit hesitant to play with frame sequences just for this issue due to the possible unexpected consequences it may have. All of the data necessary to render correctly is there (the minimum elevation is found and applied to the transform) but the rendered frame itself has this artifact, and only one sequential new frame is necessary to render it correctly. If I don't find a simple way forward today I'll leave that as a follow-up issue as it's fairly hard to reproduce it, at least from the testing I've done. |
What do you mean by "hard to reproduce"? Just curious because depending on the location, it shows up ~100% of the time for me. I think you're right though. This is still definitely an improvement 👏, so I think it's completely reasonable not to block this fix on a more general/complete fix. |
I have almost no chance of triggering it for the location you posted or any other that have similar elevation (hard refresh with network caching disabled). I'm still investigating though. Screen.Recording.2021-03-05.at.11.42.31.AM.mov |
For that location, I have almost no chance of not triggering it on hard refresh. I observe that it's pretty sensitive to the precise position (and maybe window size?). Otherwise maybe the GPU/system-dependent timing affects it? Somewhat surprising, but not ultimately urgent, I think. |
@@ -308,6 +310,7 @@ export default class DemMinMaxQuadTree { | |||
} | |||
|
|||
_addNode(min: number, max: number, leaf: number) { | |||
this.minimum = Math.min(this.minimum, min); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
Minimum among _visibleDemTiles is reduced at getMinElevationBelowMSL - this seems to track minimum among all tiles loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a leftover, this minimum is for one DEM only, after going over the mips. In getMinElevationBelowMSL we then reduce it over the visible dems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karimnaaji
Thanks. It is then redundant calculation since the minimum of a DEM tile is already available at DemMinMaxQuadTree.minimums[0].
e.g. this never asserts:
diff --git a/src/terrain/terrain.js b/src/terrain/terrain.js
index c74f6535e..b2f1beff7 100644
--- a/src/terrain/terrain.js
+++ b/src/terrain/terrain.js
@@ -747,6 +747,7 @@ export class Terrain extends Elevation {
const maxDEMError = 30.0;
this._visibleDemTiles.filter(tile => tile.dem).forEach(tile => {
const minMaxTree = (tile.dem: any).tree;
+ assert(minMaxTree.minimum == minMaxTree.minimums[0])
min = Math.min(min, minMaxTree.minimum);
});
return min === 0.0 ? min : (min - maxDEMError) * this._exaggeration;
Similar (max for tile too) reduce is done in getBounds3D where minimums[0] is used for the same purpose.
mapbox-gl-js/src/geo/transform.js
Line 996 in 74afcb2
const minmax = elevation.visibleDemTiles.reduce((acc, t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I'll update for minimums[0]
Prevent cutoff artifacts by accounting for negative terrain elevation in view frustum definition. We currently assume the minimum bound to always be mean sea level, by taking into account the minimum possible elevation of the current view, we can push the far plane further by that amount to eliminate that visual artifact.
Fixes #10317
Before / After
negative-elevation-cutoff.mov
negative-elevation-cuttoff-fix.mov
Launch Checklist
@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changesmapbox-gl-js
changelog:<changelog>Prevent cutoff artifacts by accounting for negative terrain elevation in view frustum definition</changelog>