Remove elevationOffset and fix below sea-level clipping

[birkskyum]

The first two commits just remove the elevation offset.

We have a ~450 elevation offset to move lakes below sea level to places above sea level. It's a workaroud, and it should be removed.

[github-actions]

Bundle size report:

Size Change: +132 B
Total Size Before: 204 kB
Total Size After: 204 kB

Output file	Before	After	Change
maplibre-gl.js	195 kB	195 kB	+132 B
maplibre-gl.css	9.03 kB	9.03 kB	0 B

ℹ️ View Details

No major changes

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	70.49%	14593/20702
🔴	Branches	57.26%	4899/8556
🟡	Functions	70.41%	2168/3079
🟡	Lines	71.82%	13937/19405

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟡	... / terrain.ts	79.39%	69.57%	76.92%	79.72%

Test suite run success

1605 tests passing in 127 suites.

Report generated by 🧪jest coverage report action from b0831e4

[HarelM]

@birkskyum any updates on this? Is there a way for me to help push this forward?

[birkskyum]

I don't have an update on it. I did try to adjust the far plane but always ended up with some side effects when the camera is pitched that I couldn't account for. I'm not actively working on it now.

[HarelM]

How did you test it? did you use a debug page of some sort?
Would be super cool to remove this offset as part of version 3.0...
Maybe we can start by creating tests that fail so we'd know if we managed to fix every case when all tests pass?

[birkskyum]

I used the terrain-satellite debug page and zoomed to the dead sea

[HarelM]

For me the following reproduces the error when using npm run start-debug
http://0.0.0.0:9966/test/debug-pages/terrain-satellite.html#11.1/31.5534/35.4903/97.6

I have a feeling this following line is the start of the problem:

maplibre-gl-js/src/geo/transform.ts

Line 847 in cfbc39f

this.cameraToSeaLevelDistance = this.cameraToCenterDistance + this._elevation * this._pixelPerMeter / Math.cos(this._pitch);

If I add to the elevation 450 in order to avoid negative values these artifact seems to disappear, I'm jut not 100% sure I know how this camera to sea level is used...

[HarelM]

I think I found a solution, I have no clue if it's a valid one.
@birkskyum do you want me to check it in to this branch or a different one?

[birkskyum]

Awesome! I'll try it out now

[birkskyum]

@HarelM , I think it's a good solution. I've applied the change here. Some tests fail, but overall it seems like it resolves the clipping issue without changing the elevation level.

[birkskyum]

The failing tests are all very small changes to the render output, but the benchmarks don't seem affected.

[HarelM]

I found a prettier solution I think. I'll commit it to this branch.

[HarelM]

There's still some issue, for example the following scene:
http://0.0.0.0:9966/test/debug-pages/terrain-satellite.html#13.65/31.62985/35.50558/0/57

[HarelM]

OK, now it works as expected. :-)
I've changed the logic basically only for the dead sea, other places should behave as before, no hard coded values.
I also did some micro performance improvements so that this won't degrade performance by mistake.
@birkskyum any chance you could write a test or two for this use case? Maybe even a render test and not just unit test?
@prozessor13 any change you can review this? The interesting change is in transfrom.ts class, the rest is basically the removal of the offset...

BTW, do we want to keep the ability to have an offset for exotic use cases but default it to 0?

[birkskyum]

This solution is a lot cleaner. Because no one asked for elevationOffset in the first place, I think we should nix it.

[HarelM]

@prozessor13 any chance for a quick review?
I think this is ready basically.

[birkskyum]

The only exotic use case I have for elevationOffset is actually for a render-test below sea level. We don't have hillshade data for any area below sea level, so I thought about lowering the demotiles' hill shade data.

[prozessor13]

@prozessor13 any chance for a quick review?
I think this is ready basically.

Sure, but I am on vacation, will do it at the end of this week. Regards.

[HarelM]

@prozessor13 Thanks! Enjoy your vacation!
@birkskyum I would like to have @prozessor13 review this before this is merged just to be on the safe side.

[prozessor13]

Hi,

i think this calculation in transform.ts is not in all cases correct. See:
http://localhost:9966/test/debug-pages/terrain-satellite.html#13.45/31.54164/35.33225/49.6/54
There are white areas of the dead-sea.

I think your code works when center is in the dead sea, but not when the dead sea is beside center. I created a sketch for this. I think the red part is still missing. To calculate the red part you must either know the minimum elevation in the current scene, or use a constant for this, e.g. 450m.

[HarelM]

Ah, interesting :-) I'll think about it some more...

[HarelM]

@birkskyum @prozessor13 while I don't think this fix fully solves the issue, I think it's better than now where we have the elevation offset.
There are cases as described above that still needs to be addressed, but I think this is ready to be merged to avoid conflicts in the future.
In general what I think is needed here in order to solve this, which I'm not sure I know how to do mathematically is to find out the places where the camera window crosses the terrain data - since the terrain data is not a mathematical function this makes it harder.
It's might be similar to the problem with the ray/triangle where you need to know where the two planes cross each-other.
IDK.
In any case, I think we can merge this, and try to solve the case above that @prozessor13 drew on paper in a different PR.
What do you think?

[HarelM]

Also worth taking into account that if someone would like to do a map which has elevation data for the bottom of the ocean, the hardcoded 450 won't hold. So we need to make sure the code doesn't have a hard coded value of "bottom of the dead sea" as there are DEM data that are mapping the oceans which might have a lower than -450 elevation...

[birkskyum]

A agree that this is quite important to keep working on, especially with the ocean DEM you mention, but I also see this as a step in the right direction.

[HarelM]

@birkskyum can you open a changelog change PR to add this breaking change? I forgot to add it...
Also maybe open and issue with the above diagram so that we won't forget to fix this?

[HarelM]

I've opened an issue here: #1655
So there's only a need to open a PR for changelog changes.