-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fast picking with an octree 🎉 #9961
Conversation
… (it was moved into another class here) not sure if new terrain exaggeration thing works?
Thank you so much for the pull request @DanielLeone! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
This looks great @DanielLeone! And a long time in the making too. We occasionally get high res terrain that suffers from horrible picking performance so I'm glad to see support for more than just ArcGIS terrain. It would be great to get this into an upcoming release, maybe Feb 1st. @IanLilleyT would you have a chance to review? |
I started trying this out and the performance is really good. I didn't go too deep with code review because of the big question you brought up of what to do about dynamic terrain exaggeration. Sorry for merging the dynamic terrain exaggeration PR first 😛 As you pointed out, dynamic terrain exaggeration makes this a lot more difficult because the octree "bakes" the triangles into the cells of the axis aligned box, and when the axis aligned box is exaggerated it's no longer a box shape, but curved. So now all the triangles that belong to an axis aligned cell in the unexaggerated version map to a curved cell in the exaggerated version. Maybe it would work to raycast against curved boxes, but I feel like that would be a significant performance hit from all the extra math. A lot of axis aligned optimizations would go away when exaggeration is turned on. Then I started wondering what would happen if the exaggerated curved box is converted back to an exaggerated axis aligned box. If we could find all the curved cells that overlap the axis aligned cell and loop over their triangles it should be possible to intersect the right triangle. The downside is there needs to be some sort of overlap check between axis aligned cells and curved cells. This could be made faster by converting the curved cells to oriented bounding boxes but it would mean even more false positives. Plus, converting rectangle to oriented bounding box is not the cheapest operation. Chances are this conversion would have to happen whenever the exaggeration changes. It would be nice if there were a shortcut like: the axis aligned cell overlaps at most the nearest 8 curved cells. Then the octree traversal would have to check the 8 curved cells for the current axis aligned node, but at least it should know what those are in constant time. Still cheaper than checking all cells at least. I'm not sure if such a shortcut exists. Here's a sandcastle showing the axis aligned and curved cells Bottom = not exaggerated Note how the two green dots are at the same longitude and latitude, but they are in different axis aligned cells. By checking all the curved cells that overlap the axis aligned cell (which is pretty much every curved cell in the picture), it should recover the right triangle. |
Right now I'm thinking the best chance of getting this working is to convert the exaggerated node's box to a region and find out which cells from the full exaggerated region overlap it. This would return the cells that have triangles that need to be checked. This overlap checks boils down to a region <-> region check, so it should be fast. The main slow part will be Pseudocode: When exaggeration changes:
Raycast:
Overall I'm trying to avoid the "check every triangle" slow path when exaggeration is on. If there's no good outcome we could probably merge that. But it will be pretty jarring for the user so it's not guaranteed. |
# Conflicts: # Source/Core/HeightmapTessellator.js # Source/Core/OrientedBoundingBox.js # Source/Scene/GlobeSurfaceTile.js # Source/WorkersES6/createVerticesFromQuantizedTerrainMesh.js # Specs/Core/sampleTerrainSpec.js # package.json
Hey @IanLilleyT thanks for having a look at this. Apologies for the late reply, GitHub notification settings are hard 😞 Yeah I'm happy to slowly put more time into this to see if we can get something working for terrain exaggeration too. Most of this goes straight over my head though! 😛 😢 I need more time to think about the ideas you've put down, thanks for the sandcastle as well! |
# Conflicts: # Specs/Scene/GlobeSurfaceTileSpec.js
updated access token stopped train mesh ray checking when exaggeration is applied fixed some merge conflicts
Hey guys, evidently, I unfortunately haven't been able to prioritize working on fixing the remaining issues in this PR to get it working for exaggerated terrain. Ideas beyond what Ian was already suggesting above:
If none of these are feasible - I'll probably close the PR in a couple of months, it was fun working on it :) |
…e existing tests for some reason. I can also just grab terrain exaggeration from the encoding because that is mutated on the fly. That means I don't need FrameState anymore all the way at the top
Thanks again for your contribution @DanielLeone! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
Is addressing the issue of slow globe picking on the roadmap for CesiumJS team? (Is there a public roadmap available?) It is a fairly serious performance kneecap when using non-cesium-ion terrain providers in combination with I am not skilled enough in the graphics space to push @DanielLeone 's octtree pull request forward -- but if it would be acceptable to the Cesium team to modify the branch to make his approach opt-in, I would be willing to try my hand at putting that together |
Hi 😊
(At this stage, I'd call this PR a working proof of concept)
(The code isn't quite production ready, but it's fully functional, and I think the design is open for further discussion)
(Oh, and all this code wasn't written by me, the original code and idea was from Ian :) )
What is this?
we get a nice speed up there too.
Pics
I created a new Sandcastle page to help debug this, you can go check it out if you're interested!
http://localhost:8080/Apps/Sandcastle/index.html?src=development%2FArcGIS%20Tiled%20Elevation%20Terrain%20Debug.html&label=Development
Click anywhere on the globe to draw a Ray, and then use the options on the left to show/hide the triangles/octree layers for the ray you've clicked.
Drawing the Octree at Layer 2 (well, 3rd layer, currently the deepest layer). Also drawing all triangles by their respective octree node.
You can turn on showing only the tested triangles as well. So currently in main branch, this would be every triangle. compared to this branch, only a subset is tested (orange triangles were tested)
In terms of speed-up for ArcGIS terrain, I'm getting a consistent order or magnitude
The same Sandcastle page works for Cesium World Terrain as well (first dropdown option)
and the speed-up is similar for CWT:
You can also disable the default pick strategy to feeeel the speed-up :)
How I was testing/making optimizations:
package.json
for debugging purposesrun_terrain_trace
runs a puppeteer script pointing at a pre-built sandcastle which just waits for all terrain to loadparse_traces
goes through the trace and log files recorded byrun_terrain_trace
and prints out some statistics, andrelevant logs if you call it with
--logs
.For comparison purposes, each ArcGIS terrain tile was taking about ~75ms to process (on the worker thread), and the new octree (and triangle packing) functions were contributing about 27ms of that.
If you run the two node scripts in that order, you should get a print out with similar relative results:
There's plenty of room for improvement in here as well (and a couple of Chrome deopts in the logs as well we could probably avoid). Again, not production ready code yet. Working proof of concept.
Problemos
value.
fine).
What do you want?
If anyone has a chance to have a look, let me know what you think.
My main concern is the conflict with the on-the-fly terrain exaggeration which this doesn't work for.
But at least for my use case at work, we don't use terrain exaggeration, so this would be a very welcome addition for us.
Worst case, the two algorithms I ended up with I think are nice, I'm sure they'll have some utility some day for quick octree generation and searching, even if this isn't the spot or language for them right now.
Here's a nice comparison turning the default pick strategy off at run time, and bringing us well within Chrome's frame budget while using ArcGIS terrain :)
Thanks for reading!