change internal extend to 16384 for better rendering quality #1244
Conversation
|
There were a few changes recently to the sharers including terrain 3D which are missing in the native code, so I'm not sure you be able to test the sharers. |
|
Bundle size report: Size Change: +25 B
ℹ️ View Details
|
pass round and up as own attributes. This change goes along with maplibre/maplibre-gl-js#1244
|
Just out of curiosity, what are your use-cases @candux for zoom levels higher than 14? |
this fixes all wrong patterns. Unfortunatly I really don't know where the magic number comes from
Thanks for the heads-up. I try to prepare everything in maplibre/maplibre-native#289 , so it can just be merged when the shaders in native get updated to the latest version of js.
The changes are most visible in the building. The lines are crispier and more parallel.
We are mostly interested to show more indoor details of train-stations
Yes, exactly. We want to avoid creating a new dataset just for indoor visualization which complicates the style and exporting the whole world on z15+ is quite difficult. I currently work on fixing all rendering regressions. Unfortunately there are a few places which use magic numbers which depend on the extent. Refactoring these to use the globally defined constant would be a great maintenance improvement for the future. |
makes now sense with the greater extent
|
I think it would be a good moment to put the magic number 8192 or its new value into a unchangeable global variable or something like that. |
|
One sideeffect of the greater extent would be, that the vectortiles will increase in size, because in protobuf-format bigger numbers will use more bytes. I dont know if this effect is major or minor, do you? In general i like this PR! |
|
Ah so this has en effect on memory usage you are saying @prozessor13? |
|
No, only on the protobuf vectortiles. |
it's already in https://github.com/maplibre/maplibre-gl-js/pull/1244/files#diff-27fcda492afcb31d627de1fdef3e3dd88eac4e122ed6cc2fb8fe7b4b2e0bee9f
we increased the tile extent from 4094 to 16384 for some openmaptiles layers (buildings, transportation). The size increase was only a few percents, but the quality improved a lot |
as far as I can tell all differences are because of better precision
…nt-map only minor differences because of preciser coordinates
the text below the red line is what is expected from the style. It already looks like that on main, but the diff wasn't big enough to trigger a fail. Minor changes of the position pushed to the test to fail
|
Finally, all tests pass. A lot of results had to be updated but it should always be justified because of better precision. From my perspective, this is ready to be merged. |
|
Can you post the benchmark results? I recently fixed them so maybe you need to merge main first... |
|
Please comment on the apparent difference in test/integration/render/tests/text-pitch-alignment/viewport-text-depthtest/expected.png |
|
@candux have you use one of the native platforms in the past? I would like to be able to update native when the shaders in GL JS change. See maplibre/maplibre-native#315 Any help on the native stuff would be really helpful for pull requests that change the shaders in GL JS. |
Unfortunatly they always get stuck for me at some point. This also happens on main. Doesn't matter if they run in the browser or headless. I will try it on another system.
I put the explanation in the git commit:
I can also make new PR with just that commit to update the expected results
Unfortuantly, I'm not familiar with native and only use it in tileserver-gl. I will take a look if I can help but that not really my expertise. |
|
Would be great if you could update the render test in a separate pull request. Thanks for catching this. |
|
I can give the benchmarks also a try and post them here... |
|
On a first benchmark run on my laptop, http://0.0.0.0:9966/bench/versions?compare=main#WorkerTransfer seems roughly 10 percent slower. @HarelM is that the one which is always 10 percent slower in the second run? |
|
I merged main, now the benchmarks work again... |
|
In order to avoid issues with benchmarks I run 3 version comparison, the first version is "dummy" in order to make the comparison good between the second and the third version. |
|
Where are we with this PR? What's keeping us from merging this? |
|
I wonder what the implications for native are when we change the shaders... |
|
Yes, I agree. We should discuss this in our next steering committee meeting, as the GL-JS is far ahead of the native when it comes to the shaders and we need to decide what to do there... |
Maybe this bit hack was a performance optimization. @candux what do you think, is it possible that with this pull request the rendering gets slower? |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
|
Stale or not - don't auto-close. We need to get maplibre-gl-js is updated in maplibre-gl-native, and the shaders aligned, and the gl-js benchmarks fixed, in order to get a better picture of the performance implications of this, because this visual improvement would be great to have - especially this building on the image seems to be visibly much more accurate. |
|
It's worth mentioning that native is no longer using the shaders that are here. |
|
I've been looking into rebasing this. I find this is one of these cases where we might have moved a bit too much to the style-spec, because these changes there (values should go from ~8k to 16k) in order for these tests to pass: |
if #2507 gets finished, this PR would become obsolete. But maybe this PR is still useful for the short-term. |
|
@candux interesting - if the other solution is in development, I think it's best to land it first and reassess the situation. I got reminded of this ticket again yesterday when I tried to add a point to an intersection - and it seemed like the level of accuracy needed to get it right simply wasn't supported, even though my coordinates have high precision. 
|
It's unfortunately not in active development -- I made a prototype and want to do it, but don't have available time to work on it right now. Felt needs a solution, so we're motivated to find someone to work on it, but it's still possible we'll do a server-side solution as the expedient path. |
|
@ChrisLoer , would you consider this PR an improvement to the situation until the other PR eventually lands? |
Yeah, it's an improvement -- but wouldn't fundamentally change our need for a fuller overzooming solution. If we assume "more than 6 levels of overzooming starts to look bad", then with this change it becomes "more than 7 levels of overzooming looks bad". It also doesn't help with the line-labeling issue (too many line-labels generated on overzoomed tiles, slowing things down and eventually leading to glitches or even crashes). I don't think there's any incompatibility between the two approaches though, right? One thing that occurs to me is that if we implemented #2507 with this also in place, we could get away with waiting an extra zoom level before triggering the on-the-fly tile generation, which would presumably be a perf win. |
|
@ChrisLoer We (https://geops.com/en) would be able to work on #2507 and/or finish this PR. Do you think Felt could be interested to finance this work? |
|
@kubapelc this might be of interest to you if you want to consider adding this as part of the globe implementation. |
Terminology
Background
Most MVT Datasets are limited to zoom-level 14. Generating higher zoom levels for large areas is very demanding. If you want to keep more details in MVT while staying on the same zoom-level, it is possible to increase the MVT-extent of layers for better overzooming. This became very easy to do with ST_AsMVT. Since Maplibre uses an internal extent of 8192, details of bigger MVT-Extents get lost.
PR
In this PR the internal extent of Maplibre is doubled from 8192 to 16384. This improves the quality of layers with a MVT-Extent greater 8192. This is done by using one more bit of the 16-Bit value, passed to WebGL. This bit was used so far only as an hack in the line layer, to pass additional attributes. This hack is removed in the first commit.
Screenshots
This shows the difference of main and this PR on Tiles with max-zoom 14, MVT-Extent of 16384 on Zoom-Level 21.
Before:
After:
The building is much more detailed and lines are more parallel.
Performance
Increasing the extent itself should not mess with the performance in any way.
Removing the hack in the line layer can have an impact on performance. There are two additional attributes passed to WebGL. On the other hand, the shader has a few instructions less. I wasn't able to measure any real difference because the line performance test is too noisy.
Status
This PR is at the moment not quite ready to be merged. I still have to run all tests and make the same changes in maplibre-gl-native to stay compatible with the shaders. I would like to ask for feedback, if this change is welcome and if there are more things I have to do.
Launch Checklist
maplibre-gl-jschangelog:<changelog></changelog>.