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

Add render test for 3D terrain #1320

Merged
merged 4 commits into from
Jun 24, 2022
Merged

Add render test for 3D terrain #1320

merged 4 commits into from
Jun 24, 2022

Conversation

flother
Copy link
Contributor

@flother flother commented Jun 23, 2022

This is a follow-on from PR #1316. I've had a go at creating a render test for the new 3D terrain but I can't get the test to output the correct result.

The style.json in the test takes two sources, a TerrainRGB raster-dem and a satellite raster, and adds the satellite imagery as a layer and the TerrainRGB source as 3D terrain. The map is pitched to 60°. What I would expect to see is the satellite raster draped over the 3D terrain. But what I see is a flat map with satellite imagery but no 3D terrain. You can run the test yourself by checking out this PR and running:

npm run test-render terrain/default

This will fail and the test will produce this image:

Actual test output

I'm not sure what I'm missing, so I was hoping for input from @HarelM and @prozessor13. Is there anything obviously missing from the test to get the 3D terrain in the actual result? I've tried waiting/sleeping in the style.json's metadata.test.operations, to no avail. I've also tried (unsuccessfully) to add the terrain as an operation using:

[
  "setFeatureState",
  {
    "source": "terrain",
    "exaggeration": 2,
    "elevationOffset": 50
  }
]

flother added 2 commits June 23, 2022 20:23
Tests that the root level terrain object must be in the following form:

    "terrain": {
      "source": string,
      "exaggeration": number,
      "elevationOffset": number
    }
@flother flother changed the title Covering tiles 3d Add render test for 3D terrain Jun 23, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 23, 2022

Bundle size report:

Size Change: 0 B
Total Size Before: 201 kB
Total Size After: 201 kB

Output file Before After Change
maplibre-gl.js 192 kB 192 kB 0 B
maplibre-gl.css 9.47 kB 9.47 kB 0 B
ℹ️ View Details No major changes

@HarelM
Copy link
Collaborator

HarelM commented Jun 23, 2022

I generally think you did the right thing.
The only question is if you can see the terrain in the browser if you use the same style?
A thing to remember is that render tests are running in node and not in the browser, so some stuff (maybe this one) might fail, although this might not be the cause of this issue...
I would try and see that the same style shows terrain 3D in the browser, and then add it to render tests

flother added 2 commits June 24, 2022 13:17
The original test was failing because there were no terrain tiles
available at the zoom being requested. Now it's testing at zoom 13,
which means requesting terrain tiles at zoom 12, which are available to
the tests.
The fix for #1241, commit 352bc03, ensures that elevationOffset is
taken into account when gauging which tiles are required to render 3D
terrain. If we add an extreme offset then we have a test that fails on
the current main branch but passes after the fix.
@flother
Copy link
Contributor Author

flother commented Jun 24, 2022

Thanks for the suggestion, it helped me track down the problem. It was a trivial fix in the end. I also altered the render test so that it has an explicit check for the bug described in #1241 — the terrain render test passes on this branch but fails on main.

The integration tests failed on MacOS but I don't think that has anything to do with these code changes, it just timed out. It might be that re-running it will get it to pass but I don't have permission to try that.

@flother flother marked this pull request as ready for review June 24, 2022 13:40
@wipfli
Copy link
Contributor

wipfli commented Jun 24, 2022

Started a re-run of the failing tests.
Here is a log: https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true

Run npm run test-browser
  npm run test-browser
  shell: /bin/bash -e {0}
> maplibre-gl@[2](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:2).2.0-pre.2 test-browser
> jest -c ./jest.config.e2e.js --roots ./test/integration/browser
FAIL test/integration/browser/browser.test.ts (129.[3](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:3)83 s)
  browser tests
    ✕ chromium - Drag to the left (23966 ms)
    ✓ chromium Zoom: Double click at the center (8309 ms)
    ✓ chromium - CJK Characters (12[4](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:5)6[5](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:6) ms)
  ● browser tests › chromium - Drag to the left
    thrown: "Exceeded timeout of 5000 ms for a hook.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."
      175 |     });
      17[6](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:7) |
    > 1[7](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:8)7 |     afterEach(async() => {
          |     ^
      17[8](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:9) |         await browser.close();
      17[9](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:10) |     });
      180 |
      at test/integration/browser/browser.test.ts:177:5
      at Object.<anonymous> (test/integration/browser/browser.test.ts:48:1)
Test Suites: 1 failed, 1 total
Tests:       1 failed, 2 passed, 3 total
Snapshots:   0 total
Time:        [13](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:14)0.[19](https://github.com/maplibre/maplibre-gl-js/runs/7042335260?check_suite_focus=true#step:6:20)4 s
Ran all test suites.
Error: Process completed with exit code 1.

@HarelM
Copy link
Collaborator

HarelM commented Jun 24, 2022

All tests are passing and the image looks great!
What's the deal with the last file? Those this the messages?

@flother
Copy link
Contributor Author

flother commented Jun 24, 2022

Thanks, @wipfli, that did the trick. All tests pass now.

@flother
Copy link
Contributor Author

flother commented Jun 24, 2022

@HarelM Do you mean terrain.output.json? That's another test — separate from the render test — that checks the schema for the style spec's top-level terrain object. I modelled it on the other style spec tests in test/integration/style-spec/tests. It checks that you can always use something like:

"terrain": {
  "source": "terrain",
  "exaggeration": 2,
  "elevationOffset": 2000
}

@HarelM
Copy link
Collaborator

HarelM commented Jun 24, 2022

Ahh, great! I totally missed that when I added the terrain to the style spec... Thanks!
The last question I have is if you checked that the image is partially trasparent before the fix and looks as it is now after the fix, I.e the changes in the code actually changed the render image here and fixed it.
It will be the proof that the code changes actually fixes the bug.
Regardless, this PR will be merged as it adds great tests.
Thanks again for all the hard work!

@HarelM HarelM merged commit c1cbeda into maplibre:covering_tiles_3d Jun 24, 2022
@flother
Copy link
Contributor Author

flother commented Jun 24, 2022

The last question I have is if you checked that the image is partially trasparent before the fix and looks as it is now after the fix, I.e the changes in the code actually changed the render image here and fixed it.

I did yes, and it's partly-transparent there too. Here's the (buggy) output from the render test when run on the main branch:

main

And here's the actual output after this PR:

covering_tiles_3d

@flother flother deleted the covering_tiles_3d branch June 24, 2022 15:16
@HarelM
Copy link
Collaborator

HarelM commented Jun 24, 2022

Great! Thanks!

@wipfli
Copy link
Contributor

wipfli commented Jun 25, 2022

Cool

acalcutt pushed a commit to acalcutt/maplibre-gl-js that referenced this pull request Jun 25, 2022
* Fixes maplibre#1241 - correct coveringTiles when terrain is enabled

* fix lint

* Add unit test for calculating min/max elevation (maplibre#1316)

* Add unit test for calculating min/max elevation

* Use correct method name

* Move min/max elevation value calc to Terrain class

* Alter Terrain.getMinMaxElevation to take tileID arg

* Fix unit tests after merge from main

* Fix lint

* Add render test for 3D terrain (maplibre#1320)

* Add terrain style spec integration test

Tests that the root level terrain object must be in the following form:

    "terrain": {
      "source": string,
      "exaggeration": number,
      "elevationOffset": number
    }

* Add work-in-progress render test for 3D terrain

* Fix terrain render test so it passes

The original test was failing because there were no terrain tiles
available at the zoom being requested. Now it's testing at zoom 13,
which means requesting terrain tiles at zoom 12, which are available to
the tests.

* Update terrain render test to test fix for maplibre#1241

The fix for maplibre#1241, commit 352bc03, ensures that elevationOffset is
taken into account when gauging which tiles are required to render 3D
terrain. If we add an extreme offset then we have a test that fails on
the current main branch but passes after the fix.

* Add changelog comment

Co-authored-by: Matt Riggott <flother@users.noreply.github.com>
Co-authored-by: HarelM <harel.mazor@gmail.com>
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

Successfully merging this pull request may close these issues.

3 participants