-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
Revert "[core][tile mode] Make sure only a single tile is used" #492
Conversation
This reverts commit 9ceb619. for maplibre#479 . If the tile returned is off center, this causes vector layers to be missing on the tiles that were not included
I'm still unsure why the render test in my node workflow exit with code 1 after this change, if anyone has any idea. the render test seems to work as it did before with the same number of successes/errors. on linux the exist code is 0, but when i tested in macos it did seem to get an exit code of 1 like it does in the workflow. |
I did some testing on the mac and actually found it's this 'tile-avoid-edges' test that causes the exit code 1 to happen. If I exclude "render-tests/map-mode/tile-avoid-edges" the workflow completes. It's strange though, the "actual" image generated by the "render-tests/map-mode/tile-avoid-edges" test now looks much more like the expected one...even though the test fails On another note, even if I replaced expected.png with the generated actual.png, this test does not pass. (note the "echo $?" at the end shows the last exit code. if i run it with other test the exist code is 0)
|
I, for some reason, get a lot more errors when running the tests - and not just in this PR: What commands did you run on the mac to build and run the tests? Those from node-ci, or those from DEVELOPING.md? |
I run (from node-ci), on an M1 Arm mac
|
Just a FYI, there were lots of render errors previous to this change, but they didn't cause a exit code of 1 that caused the workflow to fail. ex, in main right now
|
I made a few updates to what I remember running. I used the node workflow to get the commands too. One thing I notice is I chose "Release" build type. I wonder if Debug would give more info. I have been testing the main change in this, removing the single tile restriction and it does for sure fix this issue it tileserver. This is really the one thing holding back a release of that. I am a bit annoyed the test I used to troubleshoot this had to be disabled though. I tried a few things to update the test, like updating the image and manifest, but the test still failed (even though the image it makes looks good) |
I noticed the two images you posted look identical but have much different file sizes. I wonder if there is a hint. |
The CI for ios is blocked until we merge this #494 |
When I was looking through the commits for this, I noticed they had also updated the expected image and metrics in this commit. 8f8b7af . So it does seem the outcome of a single tile being displayed in tile mode was expected. We don't have that updated image in the maplibre-gl-js, so it is comparing against the original. In tileserver-gl where I am having this issue, I decided to look again at creating a separate pool of renderers in static mode. I made this update which didn't require as many changes as I thought it would. maptiler/tileserver-gl#608 Now I am unsure if I could continue this PR or just fix it in tileserver. the original 9ceb619 says it was for "various symbol placement problems like, symbol duplications or cut-offs, memory/cpu overhead, so maybe I should just close this and rework the tileserver side. |
This reverts commit 9ceb619. for #479 .
If the tile returned is off center, this causes vector layers to be missing on the tiles that were not included