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 Unreal API for terrain height querying #1421

Merged
merged 71 commits into from
Sep 27, 2024
Merged

Add Unreal API for terrain height querying #1421

merged 71 commits into from
Sep 27, 2024

Conversation

csciguy8
Copy link
Contributor

@csciguy8 csciguy8 commented May 13, 2024

Builds on cesium-native PR 783, so merge that first.

Introduces new test to verify results and measure performance of new cesium-native terrain height query functionality.
(using the existing performance test framework)

Still a bit of work to do here...

image

@kring kring marked this pull request as ready for review September 19, 2024 05:04
@kring
Copy link
Member

kring commented Sep 19, 2024

This is ready to review!

It's taking ~30 seconds to generate distance fields for built-in meshes
(not our tiles) during automated test runs on CI, and if this happens at
the wrong time it can cause async tests to time out and fail. E.g.:

```
[2024.09.19-13.55.42:741][157]LogMeshUtilities: Finished distance field build in 14.6s - 126x126x126 sparse distance field, 1.9Mb total, 0.1Mb always loaded, 52% occupied, 1322 triangles, EditorSkySphere
[2024.09.19-13.55.57:236][896]LogMeshUtilities: Finished distance field build in 14.3s - 126x126x126 sparse distance field, 1.9Mb total, 0.1Mb always loaded, 52% occupied, 1322 triangles, SM_SkySphere
[2024.09.19-13.55.58:335][ 28]LogMeshUtilities: Finished distance field build in 1.0s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 16 triangles, Cube
[2024.09.19-13.55.59:518][170]LogMeshUtilities: Finished distance field build in 1.2s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 320 triangles, Sphere
[2024.09.19-13.56.00:710][313]LogMeshUtilities: Finished distance field build in 1.0s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 170 triangles, Cylinder
[2024.09.19-13.56.05:147][845]LogAutomationController: Error: Test Completed. Result={Fail} Name={works with a single position} Path={Cesium.Unit.SampleHeightMostDetailed.Cesium World Terrain.works with a single position}
```
@kring
Copy link
Member

kring commented Sep 20, 2024

Or... not quite ready for review. The new tests are taking an extraordinarily long time on CI - over 30 seconds when they just take a few milliseconds locally! My best guess is that the Cesium3DTileset is either not ticking, or it's ticking much more slowly, and so the height queries are taking ages.

@kring
Copy link
Member

kring commented Sep 23, 2024

The test failures in UE 5.3 and 5.4 are caused by this previously-discovered problem:
#1219
If Actors don't tick, then terrain doesn't load. Not sure what best to do about this yet.

@kring
Copy link
Member

kring commented Sep 25, 2024

The problem mentioned above has been fixed, so this is ready to review.

@j9liu
Copy link
Contributor

j9liu commented Sep 26, 2024

This seems to work well! I'm still adding some comments but the results look great so far. In our 04_MAIN_CesiumSublevels level I tested putting a globe-anchored in every location, which involved querying against both Cesium World Terrain and Aerometrex Denver Photogrammetry. Both queries completed and resulted in accurate placement on each tileset -- nice!

Also tested what happens if you give the function a position that isn't on the tileset, and it correct returns false for the sample's success.

Here's the Blueprint I used if anyone wanted it for reference:
https://blueprintue.com/blueprint/t0mh0s4t/

I also noticed that the performance tests take between 6-10 seconds, is that expected? I thought it might have to do with the cache, but even with a warm cache it took around 5-6 seconds on my machine. Maybe I had too many browser tabs open 😛

Copy link
Contributor

@j9liu j9liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! I didn't pore over the changes to the test code, but I verified that the tests pass on my machine and that the behavior works.

My comments are mainly doc / naming tweaks. To speed things up for the release, I'm going to implement most of them myself, but I will leave the open-ended ones up in case you have opinions @kring .

@kring
Copy link
Member

kring commented Sep 27, 2024

I also noticed that the performance tests take between 6-10 seconds, is that expected?

That doesn't sound too far out of line. The tests are doing 400 height queries, spread out over a large enough area that it requires loading quite a few tiles, especially in the GP3DT case.

@kring
Copy link
Member

kring commented Sep 27, 2024

I think I've addressed everthing, this is ready for another look.

@j9liu
Copy link
Contributor

j9liu commented Sep 27, 2024

@kring the changes look good, but I'm still seeing the same behavior with the tileset not being valid. :(

I set the ion Asset ID of a tileset to be 0, which is not a valid tileset. I stepped through the code and found that pTileset is actually not null, so it's still attempting to resolve the height queries in the tileset's updateView. Then I think it's an issue with cesium-native. In particular, this line is never triggering:

    if (!this->_heightRequests.empty() && this->_pTilesetContentManager &&
        this->_pTilesetContentManager->getRootTileAvailableEvent().isReady()) {
      TilesetHeightRequest::failHeightRequests(
          this->_heightRequests,
          "Height requests could not complete because the tileset failed to "
          "load.");
    }

_heightRequests isn't empty, and _pTilesetContentManager isn't nullptr. But I guess that getRootTileAvailableEvent isn't being marked as ready. Is that expected? I guess if there's no root tile, then it doesn't make sense to have that event be ready, so maybe we should remove this check?

@j9liu
Copy link
Contributor

j9liu commented Sep 27, 2024

Other than that, this can be merged! If it can't be fixed before release, no worries, we can note it as a TODO issue in cesium-native.

@j9liu j9liu merged commit f032377 into main Sep 27, 2024
23 checks passed
@j9liu j9liu deleted the terrain-query-test branch September 27, 2024 19:35
@kring
Copy link
Member

kring commented Sep 27, 2024

@j9liu I believe asset ID 0, and also a blank URL, are a special case because we never even try to load a tileset in those states. Any other tileset failure should work as expected, I think. I can't quite decide if the behavior in the special case is a bug or a feature... Essentially it'll wait until those properties are set and then do the height query, rather than failing immediately.

@j9liu
Copy link
Contributor

j9liu commented Sep 27, 2024

Ah okay @kring . I was also trying other bogus asset IDs aside from 0 (like 2, which is a raster overlay), but I can see them falling into the same category of "we never load this tileset". These were just the easiest "failures" I could simulate, that's why I seem fixated on them 😛 If there's any other failures I should / could test, definitely let me know

@kring
Copy link
Member

kring commented Sep 27, 2024

Asset ID 2 should fail. If it's not, something is wrong. I'll check it out!

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