Hillshade with map-anchor changes

[cns-solutions-admin]

If the hillshade "hillshade-illumination-anchor": "map" the shading should not change, when rotating, changing the view. However, at some bearings/pitches it does: it looks, as if the shading is applied twice?

maplibre-gl-js version: 2.2.0

browser: Chrome, Firefox

Steps to Trigger Behavior

1. rotate the map with terrain and hillshading, esp. with high pitch (60°).

Link to Demonstration

• https://jsfiddle.net/martinatcns/c91br8tv/59/

Here are screenshots with minimal different bearing (at max pitch) - one looks smoother than the other:

Here is an extreme example of above fiddle:

Expected Behavior

Hill shading does not change when rotating/pitching

Actual Behavior

At some bearings/pitches the hillshading changes.

[prozessor13]

thx for the bugreport!

I can reproduce the issue, after some other issues i will look onto that!

[cns-solutions-admin]

I got the fiddle working and added another screenshot from it, where the differences are in the same view.

[prozessor13]

After a long time digging into to this i found the problem:

The render to texture framebuffer has no stencil flag setted. With the stencil logic enabled, in hillshade and raster layers, every pixel is rendered only with the data from the highest available tile.

https://github.com/maplibre/maplibre-gl-js/blob/main/src/render/terrain.ts#L254 https://github.com/maplibre/maplibre-gl-js/blob/main/src/render/draw_hillshade.ts#L28

Theoretically this is only setting somewhere the gl.DEPTH_STENCIL_ATTACHMENT flag. But because all GL logic is encapsulated in classes, and because this may implact performance i think it will take some time to implement this. Currently i do not have the time, so is someone willed to fix this issue?

[prozessor13]

Hello,

i implemented a basic stencil logic when rendering to texture. Can you please test your issues with this branch: https://github.com/prozessor13/maplibre-gl-js/tree/rtt_stencil

Thx.

[HarelM]

The best approach would be to write a test to simulate this issue somewhere and make sure this test is solved. Render tests are probably the best approach. @cns-solutions-admin Any change you can try and write a test that demonstrate the issue? Maybe open a PR with that test so that @prozessor13 can take that test to his branch and know that the solution he wrote fixes this issue?

[cns-solutions-admin]

It's much better know.

There might be still a problem with tilting and/or distance of the tile: When tilting a bit more the far tiles get darker:

[cns-solutions-admin]

Regarding testing: as I don't know exactly how it should look like and how to create a correctly rendered image, I don't see how we could create a test now.

As for testing: the following is the same as the fiddle - extract the html file and add your maplibre-gl.js/css: index.zip

[HarelM]

You can create a test now of the issue - i.e. a render test of the above scene for example as ot is with the bug. If the bug gets solved we can see that the test fails (because it was changed) and we can compare the images. If the new image is acceptable we have a test to cover this scenario and we can see how it was improved when coming to review the code.

[prozessor13]

This can have 3 reasons:

• The rtt_stencil branch from my repo depeds on the old paralell_rrt branch of my repository. This had some issues, which are already fixed. I created a PR https://github.com/maplibre/maplibre-gl-js/pull/1672. Please try this.
• In the terrain-osm.html debug page the hillshade layer has setted hillshade-illumination-anchor to viewport. Because of changing logic in paralell_rrt2 to render hillshade to texture, and because of rtt-tiles will be cached, this can also create issues, but can be fixed with hillshade-illumination-anchor setted to map.
• When tilting the map more and more, the LOD (=decreasing zoomlevel the more the tile is away from camera) gets more into action, so there is a point where lower-res tiles gets in place (you can see this effect as well on the osm tiles). Because of lower res, the shillshading also looks a bit different.

[HarelM]

This is generally solved by #1672. Assigned a S bounty here to write some tests to cover the changes in the branch and this issue. Link to parent Bounty: https://github.com/maplibre/maplibre/issues/189

[tempranova]

OK, I can try taking on writing these tests. I've gotten part of the way so far. @HarelM , do you have a specific set of tests you'd like? You mention one above, which is essentially testing for the error described above. I'm regarding this test as "see if you can cause the weird rendering error to happen, write a render test that catches this error, then see if the new branch solves it and rewrite the test for the new branch". Are there any specific cases that you would like other tests for, or just this general issue where the terrain renders oddly on occasion while changing rotation at high pitch?

[HarelM]

Yes, render tests would be ideal, but if this can't be easily reproduced in render tests, then unit tests would be enough.

[tempranova]

Update... finally have this error occurring in a render test. Going to clean it up in the next couple days and then test it with the new changes to see if that solves the issue!

[HarelM]

Great! thanks for the info! Note that the branch is here in this repo so you can open a PR against that branch, once merged we can merge the branch here to main. Keep me posted on the progress as this is an important part of version 3.0 release.

[tempranova]

OK! When I run the test in the rtt_stencil branch (#1672) it looks like the test comes out cleanly. Here's an example from the current main:

And from the rtt_stencil branch:

Definitely there is a difference in the render here and it looks like it doesn't "overlap" in the updated branch. Yay!

@HarelM , you mention a few things in this comment about making a test that would fail, and then using that to verify that the test is successful? Obviously I can just move over the test I made into the new branch, and use the image to match that it works, but you mention doing this sort of "failure to test success". Do you want me to just move in the test as-is or try to create a scenario where we can still see the old "buggy" version of the render as well? Hope that's not too confusing a question.

Once I know just how you want the test to work, then I'll clean it up and try to reduce tiles required to the absolute minimum and then get it up in a PR.

[HarelM]

The test you created above is enough. If you put the second image as the expected file the test fail in main and using the branch will cause the test to pass. Would be great if you can have a unit test as well, although I'm not sure it this unit test has a lot of value - so it's up to you. Thanks for the update! Reducing bounty to M as I've seen tests are harder to write than I remembered.

[tempranova]

Okey dokey! I trimmed a whole bunch of the tiles I was including to keep enough to cause the test to work, but not quite as much bloat. I'm sure it could be more optimized (ie, the sleep reduced slightly, maybe a couple less tiles loaded), but I think it's reasonable at this point. Let me know any more fixes desired.

The PR is at #2414

[wipfli]

Thanks for fixing this issue, @tempranova. Is it correct that this is your invoice on OpenCollective? https://opencollective.com/maplibre/expenses/134750

[wipfli]

We ask people to doubly link invoice->issue and issue->invoice because sometimes project get scam invoices on OpenCollective...

[lseelenbinder]

@tempranova I'd love to get this bounty payment approved for you! Could you confirm that https://opencollective.com/maplibre/expenses/134750 is the correct invoice? Thanks!

[tempranova]

Yes, that's right! Sorry I didn't see the previous comments. But that invoice (https://opencollective.com/maplibre/expenses/134750) is the correct one.

[lseelenbinder]

Thanks @tempranova! Should be paid out soon.