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.

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

Labels getting clipped in tile mode (node version) #284

Closed
acalcutt opened this issue May 19, 2022 · 22 comments
Closed

Labels getting clipped in tile mode (node version) #284

acalcutt opened this issue May 19, 2022 · 22 comments

Comments

@acalcutt
Copy link
Collaborator

acalcutt commented May 19, 2022

I mentioned this issue in #279 , but did not want to pollute that thread with an unrelated problem.

In my testing of the node version of maplibre-gl-native in my branch of tileserver-gl I have noticed labels getting clipped more than in the mapbox-gl-native version.

For example, in the maplibre-gl-native version it is consistently cutting off labels all over the place like this
maplibre

Where in the maptiler version using mapbox-gl-native v5.02, does not have this issue
mapbox

Looking at several similar threads I found these related mapbox issues
mapbox/mapbox-gl-native#15805
mapbox/mapbox-gl-native#14011
mapbox/mapbox-gl-native#15880
The first article in this list mentions this was improved in v5.0.2, which I did find the change they mentioned here mapbox/mapbox-gl-native@d500b67. the padding was increased when in tile mode to help the labels to help with the trimming (a change which still exists, slightly rewritten). Also, in tileserver-gl they added the code to use tile mode here maptiler/tileserver-gl@5048388

Looking though the related code in https://github.com/maplibre/maplibre-gl-native/blob/main/src/mbgl/text/collision_index.cpp#L26-L50 it seems like the changes that added padding in tile mode are still there and looking in my tileserver-gl code the tile mode option is still set ( https://github.com/acalcutt/tileserver-gl/blob/main/src/serve_rendered.js#L574 )

From this, to me, it seems like it should be showing the labels correct like in the second screenshot, but for some reason it is not.

One idea I had is maybe it is ignoring the tile mode switch? I'm guessing that would be this code in the node binding code?

Does anyone have any other idea or tips that could help? I am told there is no issue with labels in the android version, which Is why I was thinking something with the node mode switch.

@acalcutt acalcutt changed the title Labels getting clipped in node tile mode (node version) Labels getting clipped in tile mode (node version) May 19, 2022
@wipfli
Copy link
Member

wipfli commented Jun 4, 2022

Thanks for reporting this @acalcutt Is there a way to reproduce this cropping problem with the linux native renderer and the demotiles?

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jun 4, 2022

Is the any information on using the linux native renderer? like at least switches mbgl-render needs?

Basically I would want to try an recreate this view, where it trims "United States"
https://tiles.wifidb.net/styles/demotiles/?raster#6/39.470/-98.701
image

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jun 4, 2022

I tried various zoom levels, like
xvfb-run -a ./mbgl-render -z 6 -y 39.436 -x -101 --style style.json --output out6.png

but I wasn't able to see any clipping. I always assumed this was where the label was at the edge of the tile. so I think to test this you would need two tiles that are side by side where the label gets clipped off. also, I'm not sure this mbgl-render can do tile mode, where it overdraws a bit to help with the labels.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jun 5, 2022

I've made a application to test this issue here
https://github.com/acalcutt/maplibre-label-clipping-test
( Note, this is made to be run in node 10 to be compatible with the mapbox-gl-native release )

Test 1 - With maplibre-gl-native
git clone https://github.com/acalcutt/maplibre-label-clipping-test.git
cd maplibre-label-clipping-test
npm i
xvfb-run -a node index.js
2 images are generated, which look like this. I added the red boxes to show where the issue is.
ml_sel

Test 2 - With mapbox-gl-native
Edit index.js.
uncomment line 8 (let mbgl = require('@mapbox/mapbox-gl-native');)
comment line 9 (//let mbgl = require('@acalcutt/maplibre-gl-native');)
xvfb-run -a node index.js
2 images are generated, which look like this.
mb_sel

Compare the maplibre version to the mapbox version. In the maplibre version, the "North" in "North Adams", which is right on the edge, is missing. However in the mapbox version, it is there as expected.
mlmbcombined

@wipfli
Copy link
Member

wipfli commented Jun 5, 2022

I also tried with

wget https://raw.githubusercontent.com/maplibre/demotiles/gh-pages/style.json
./build/bin/mbgl-render --style style.json --output out.png --zoom 6 --lat 39.470 --lon -103

but always got correctly clipped labels...

@wipfli
Copy link
Member

wipfli commented Jun 5, 2022

Hm interesting, is the diff between mapbox-gl-native and maplibre-gl-native large? When was their last common commit?

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jun 5, 2022

The diff is pretty large between the mapbox last version.

My assumption right now is this is related to the mode switch. If I go remove this line (mode: 'tile',) https://github.com/acalcutt/maplibre-label-clipping-test/blob/main/index.js#L147 , with the mapbox version, it produces the same output as maplibre.

The code for tile mode has been refactored a bit, since mapbox 5.0.2...maybe there was some mistake with that
https://github.com/maplibre/maplibre-gl-native/blob/main/src/mbgl/text/collision_index.cpp#L26-L50
or maybe something reading the mode setting?
https://github.com/maplibre/maplibre-gl-native/blob/main/platform/node/src/node_map.cpp#L1403-L1409
1
2

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jun 7, 2022

I've done a few thing to try and track this down...non successful so far.

1.) I tried setting viewportPaddingDefault to 1024, like is supposed to happen in tile mode to see if maybe the tile mode switch was not working. However even at 1024 the label clipping still occurred.

2.) I tried reverting back to the first maplibre node release in cfcbc62 , but the issue seemed to be happening in that release also

3.) I tried going back really far to a mapbox release that mentioned tile mode b84449e , but unfortunately I couldn't get it to build. I used the instruction for that release, but I assume it needs an older GCC since I was getting syntax errors.

I've been trying to test this with https://www.npmjs.com/package/@acalcutt/maplibre-gl-native-test since I wasn't sure how to use an unreleased version of maplibre-gl-native with node. does anybody know if that is possible (to use the node package without releasing it?)

@wipfli
Copy link
Member

wipfli commented Jun 7, 2022

Maybe you can install it from a local folder https://stackoverflow.com/questions/8088795/installing-a-local-module-using-npm

@pbnsilva
Copy link

Have you made any progress @acalcutt ? I'm seeing a fair amount of clipping and overlaps.

@acalcutt
Copy link
Collaborator Author

Unfortunately I haven't figured anything else. I haven't had much time to look though since my real job has kept me really busy this month.

My last test leads me to believe maybe it isn't tile mode padding like I was originally thinking.

@pbnsilva
Copy link

Thanks for putting in the work. I'll do a bit of digging myself this week.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jul 5, 2022

I updated @acalcutt/maplibre-gl-native@5.0.21 to include these render fixes. unfortunately it doesn't look like they fixed the issue here.

I was hopeful since the fixes were in the same files... but somewhat expected it since I had tried to revert #270 before and it didn't fix this issue.

I also updated the test app in this thread to use the latest node release and confirmed it didn't help this issue.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Jul 5, 2022

I will say I need to look at these render test in #351 (comment)

'render-tests/text-variable-anchor/all-anchors-tile-map-mode' looks like it tests this.

@wipfli
Copy link
Member

wipfli commented Jul 5, 2022

Maybe also look in the ignored tests.

If I run

cmake . -B build -G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=gcc-10 -DCMAKE_CXX_COMPILER=g++-10
cmake --build build -j $(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null)
./build/mbgl-render-test-runner --manifestPath metrics/linux-clang8-release-style.json &> render-test.log
sed -i 's/\x1b\[[0-9;]*m//g' render-test.log
cat render-test.log | grep "* ignore"

I get these ignored tests:

* ignore render-tests/symbol-cross-fade/chinese (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/real-world/bangkok (https://github.com/mapbox/mapbox-gl-native/issues/10412)
* ignore render-tests/real-world/sanfrancisco (https://github.com/mapbox/mapbox-gl-native/issues/10412)
* ignore render-tests/real-world/chicago (https://github.com/mapbox/mapbox-gl-native/issues/10412)
* ignore render-tests/tilejson-bounds/overwrite-bounds (started failing after https://github.com/mapbox/mapbox-gl-native/pull/16091)
* ignore render-tests/fill-extrusion-vertical-gradient/default (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-vertical-gradient/false (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/mixed-zoom/z10-z11 (https://github.com/mapbox/mapbox-gl-native/issues/10397)
* ignore render-tests/projection/perspective (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/runtime-styling/set-style-glyphs (started failing after https://github.com/mapbox/mapbox-gl-native/pull/16091)
* ignore render-tests/fill-extrusion-geometry/linestring (https://github.com/mapbox/mapbox-gl-native/pull/14240)
* ignore render-tests/regressions/mapbox-gl-js#6806 (pending https://github.com/mapbox/mapbox-gl-js/pull/6812)
* ignore render-tests/regressions/mapbox-gl-js#5740 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/regressions/mapbox-gl-js#6706 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/regressions/mapbox-gl-js#7271 (https://github.com/mapbox/mapbox-gl-native/issues/12888)
* ignore render-tests/regressions/mapbox-gl-js#2762 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/regressions/mapbox-gl-js#5642 (Failing with mbgl-render-test)
* ignore render-tests/regressions/mapbox-gl-native#7357 (https://github.com/mapbox/mapbox-gl-native/issues/7357)
* ignore render-tests/regressions/mapbox-gl-js#5982 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/regressions/mapbox-gl-js#2769 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/regressions/mapbox-gl-js#7066 (Failing with mbgl-render-test)
* ignore render-tests/regressions/mapbox-gl-js#2467 (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/symbol-sort-key/placement-tile-boundary-right-then-left (https://github.com/mapbox/mapbox-gl-js/pull/9054)
* ignore render-tests/text-max-width/zero-width-point-placement (https://github.com/mapbox/mapbox-gl-native/issues/15648)
* ignore render-tests/extent/1024-circle (needs investigation)
* ignore render-tests/extent/1024-symbol (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/icon-text-fit/text-variable-anchor-overlap (https://github.com/mapbox/mapbox-gl-native/issues/15809)
* ignore render-tests/zoomed-fill/negative-zoom (https://github.com/mapbox/mapbox-gl-native/issues/16019)
* ignore render-tests/geojson/inline-linestring-fill (current behavior is arbitrary)
* ignore render-tests/debug/padding/ease-to-no-distort (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/padding/set-padding (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/padding/ease-to-right-distort (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/padding/ease-to-left-distort (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/padding/ease-to-btm-distort (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/padding/ease-to-top-distort (https://github.com/mapbox/mapbox-gl-native/issues/16212)
* ignore render-tests/debug/collision (https://github.com/mapbox/mapbox-gl-native/issues/3841)
* ignore render-tests/debug/tile-overscaled (https://github.com/mapbox/mapbox-gl-native/issues/3841)
* ignore render-tests/debug/overdraw (https://github.com/mapbox/mapbox-gl-native/issues/15638)
* ignore render-tests/debug/raster (https://github.com/mapbox/mapbox-gl-native/issues/15510)
* ignore render-tests/debug/tile (https://github.com/mapbox/mapbox-gl-native/issues/3841)
* ignore render-tests/line-pattern/opacity (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/15320)
* ignore render-tests/line-pattern/with-dasharray (https://github.com/mapbox/mapbox-gl-js/pull/9189)
* ignore render-tests/fill-opacity/zoom-and-property-function-pattern (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/15322)
* ignore render-tests/raster-masking/overlapping-zoom (https://github.com/mapbox/mapbox-gl-native/issues/10195)
* ignore render-tests/text-size/nan (https://github.com/mapbox/mapbox-gl-native/issues/16020)
* ignore render-tests/background-color/colorSpace-hcl (needs issue)
* ignore render-tests/background-color/transition (https://github.com/mapbox/mapbox-gl-native/issues/10619)
* ignore render-tests/line-translate/literal (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/14859)
* ignore render-tests/tile-mode/streets-v11 (ignored due to flaky metrics results)
* ignore render-tests/fill-pattern/update-feature-state (https://github.com/mapbox/mapbox-gl-native/issues/15895)
* ignore render-tests/fill-pattern/literal (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/14423)
* ignore render-tests/fill-pattern/zoomed (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/14768)
* ignore render-tests/fill-pattern/opacity (Flaky on Linux: https://github.com/mapbox/mapbox-gl-native/issues/14870)
* ignore render-tests/fill-extrusion-translate/default (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-pattern/feature-expression (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/literal (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/opacity (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/function (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/function-2 (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/tile-buffer (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/fill-extrusion-pattern/@2x (https://github.com/mapbox/mapbox-gl-js/issues/3327)
* ignore render-tests/feature-state/promote-id-fill-extrusion (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/feature-state/promote-id-line (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/feature-state/promote-id-fill (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/feature-state/promote-id-circle (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/feature-state/promote-id-symbol (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-base/property-function (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-base/zoom-and-property-function (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/collator/default (Some test platforms don't resolve 'en' locale)
* ignore render-tests/collator/resolved-locale (Some test platforms don't resolve 'en' locale)
* ignore render-tests/fill-extrusion-opacity/default (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-opacity/literal (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore render-tests/fill-extrusion-opacity/function (https://github.com/mapbox/mapbox-gl-js/pull/9202)
* ignore query-tests/regressions/mapbox-gl-js#7883 (https://github.com/mapbox/mapbox-gl-native/issues/14585)
* ignore query-tests/regressions/mapbox-gl-js#8999 (https://github.com/mapbox/mapbox-gl-js/pull/9138)
* ignore query-tests/fill-translate/multiple-layers (https://github.com/mapbox/mapbox-gl-native/issues/12701)
* ignore query-tests/fill-extrusion/sort-rotated (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/sort (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/top-in (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/sort-concave-outer (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/box-in (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/sort-concave-inner (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/side-in (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/fill-extrusion/base-in (https://github.com/mapbox/mapbox-gl-native/issues/13139)
* ignore query-tests/geometry/polygon (needs investigation)
* ignore query-tests/geometry/multipolygon (needs investigation)
* ignore query-tests/geometry/multilinestring (needs investigation)
* ignore query-tests/evaluated/line-width (Port https://github.com/mapbox/mapbox-gl-js/pull/9198)
* ignore query-tests/fill-extrusion-translate/multiple-layers (https://github.com/mapbox/mapbox-gl-native/issues/12701)

@acalcutt
Copy link
Collaborator Author

acalcutt commented Aug 27, 2022

I think I may have figured this out, but I am still testing to be sure.

For some reason in platform/node/src/node_map.cpp , the map "withMapMode" was hardcoded to "mbgl::MapMode::Static" and not using the "mode" variable
L1424 and L645

@acalcutt
Copy link
Collaborator Author

acalcutt commented Aug 27, 2022

It worked! So far I have tested in my tileserver-gl and in the label clipping test above and it seems like it is fixed

This should be fixed in my npm package
"@acalcutt/maplibre-gl-native": "5.0.25

This is what the two images from my label test look like now, no longer clipping labels
1

@wipfli
Copy link
Member

wipfli commented Aug 28, 2022

Really nice! Do you know if we can run the render test via the node platform?

@birkskyum
Copy link
Member

Fixed by #415

@acalcutt
Copy link
Collaborator Author

@wipfli unfortunately I am not aware of how to run the render tests for node, or if there are any.

@acalcutt
Copy link
Collaborator Author

acalcutt commented Aug 28, 2022

@wipfli

To do the basic tests the build does in linux, it would be something like ubuntu 20.04

install prereqs from https://github.com/acalcutt/maplibre-gl-native/tree/main/platform/linux
git clone --recurse-submodules https://github.com/maplibre/maplibre-gl-native.git
npm ci --ignore-scripts
cmake . -B build -G Ninja -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=gcc-10 -DCMAKE_CXX_COMPILER=g++-10
cmake --build build -j $(nproc 2>/dev/null || sysctl -n hw.ncpu 2>/dev/null)
xvfb-run --auto-servernum npm test

however, looking in the package.json there are some other tests that didn't seem to work for me on my test vm

    "test-memory": "node --expose-gc platform/node/test/memory.test.js",
    "test-expressions": "node -r esm platform/node/test/expression.test.js",
    "test-render": "node -r esm platform/node/test/render.test.js",
    "test-query": "node -r esm platform/node/test/query.test.js",

Edit: the error test-render gets after installing d3 is

/opt/test6/maplibre-gl-native/maplibre-gl-js/test/integration/lib/harness.js:1
Error [ERR_REQUIRE_ESM]: require() of ES Module /opt/test6/maplibre-gl-native/node_modules/d3/src/index.js not supported.
Instead change the require of index.js in null to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/opt/test6/maplibre-gl-native/maplibre-gl-js/test/integration/lib/harness.js:1)
    at Generator.next (<anonymous>)
    at Object.<anonymous> (/opt/test6/maplibre-gl-native/maplibre-gl-js/test/integration/lib/render.js:1)
    at Generator.next (<anonymous>)
    at Object.<anonymous> (/opt/test6/maplibre-gl-native/platform/node/test/render.test.js:1)
    at Generator.next (<anonymous>) {
  code: 'ERR_REQUIRE_ESM'
}

seems like it runs the render test from the js version?

@wipfli
Copy link
Member

wipfli commented Aug 29, 2022

Ah interesting. The render test fixtures are indeed in the maplibre-gl-js submodule. Maybe even the test harness from GL JS is used for node?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants