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

Visit terrain tiles depth first and near-to-far #4616

Merged
merged 23 commits into from
Nov 11, 2016
Merged

Visit terrain tiles depth first and near-to-far #4616

merged 23 commits into from
Nov 11, 2016

Conversation

kring
Copy link
Member

@kring kring commented Nov 7, 2016

This builds on #4613 so merge that first.

This changes QuadtreePrimitive to traverse tiles depth first (instead of the breadth first traversal it used before) and in approximate near-to-far order. Tiles are queued for load in the traversal order, which means that nearer tiles will be loaded first.

There's no user-visible change, and I don't really see any performance difference (unlike #4613 which takes only 3/4 of the time to load the Mt Everest scene versus master, in my tests today!), so I didn't update CHANGES.md. Just consider this refactoring.

The approximate near-to-far order is interesting.... it just uses the natural ordering of the quadtree, not actual distances to tiles. That's fine for culling; a "closer" quadtree node will never be occluded by a "farther" one. But it's still possible for a sub-tile in a "farther" quadtree node to actually be closer to the viewer than a sub-tile in a "closer" quadtree node. Closer, but never in front of. Hopefully that explanation makes some sense.

The end result of this is that tiles don't perfectly appear near-to-far, they only sorta do. But that's probably as close as we'll get without explicitly sorting by distance.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Part of #526 (@kring if you have a minute, do you want to update that roadmap since some work has been done?).

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

unlike #4613 which takes only 3/4 of the time to load the Mt Everest scene versus master, in my tests today!

Can you put that in CHANGES.md? We'll also say so on twitter and in the release blog post. This is important to many users so we should get the word out.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

I don't really see any performance difference

Just curious, did you profile before and after? I'm sure this saved some CPU time and moved around the CPU bottlenecks (hopefully in a useful way).

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Tiles are queued for load in the traversal order, which means that nearer tiles will be loaded first.

They are also rendered in this order, right? But they were also previously sorted front-to-back for rendering, right?

The reason I ask is early-z. Assuming we weren't sorting front-to-back and the fragment shader doesn't discard or do anything strange, I would expect a performance win in the steady state (no tiles pending load) and for views with high depth complexity (horizon views) and expensive shaders (water).

By the way, good article on this: http://casual-effects.blogspot.com/2013/08/z-prepass-considered-irrelevant.html

}

function queueChildLoadNearToFar(primitive, cameraPosition, southwest, southeast, northwest, northeast) {
if (cameraPosition.longitude < southwest.east) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine if you want to keep it like this with explicit SW, SE, NW, and NE properties, but I believe the way the orignial paper implemented this was to have an array of 4 children (could have enums for SW, SE, NW, and NE), and several arrays of indices (in the range 0 to 3) for each sort order. This function would then just select the right array of indices based on the camera position.

Up to you but worth considering.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Is it possible to add reasonable unit tests to verify the sort order and perhaps tests for the new QuadtreePrimitive properties? I believe 3D Tiles has tests like this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

@lilleyse can you also review this? It's a good example of how when we know the exact spatial data structure, we can do specific optimizations. I'm not sure that we would implement exactly this in 3D Tiles when we have explicit tiling schemes, but it will still be useful to review.

The same pre-sort is also possible with octrees, of course.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Does the QuadtreePrimitive Sandcastle example need any updates?

@kring
Copy link
Member Author

kring commented Nov 8, 2016

They are also rendered in this order, right? But they were also previously sorted front-to-back for rendering, right?

Yeah, they were already rendered near-to-far via an explicit sort. Thanks for the reminder that I can remove that explicit sort, though.

@kring
Copy link
Member Author

kring commented Nov 8, 2016

I believe the way the orignial paper implemented this was to have an array of 4 children (could have enums for SW, SE, NW, and NE), and several arrays of indices (in the range 0 to 3) for each sort order.

What I did should be slightly faster because it involves less jumping around in memory. I didn't measure, that's just a semi-educated guess. IMO it's clearer, too.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 8, 2016

What I did should be slightly faster because it involves less jumping around in memory. I didn't measure, that's just a semi-educated guess. IMO it's clearer, too.

I don't know that it is more clear, but, yes, I suspect it has better cache coherence. I tried to think about this more, e.g., the array is so small and if the loop is tight enough it is likely to stay in L2, but then I remembered that this is JavaScript and I have no idea. Leaving it is perfectly OK.

@kring
Copy link
Member Author

kring commented Nov 10, 2016

cesium-load-order-improvements

Master is on the left, this branch is on the right. Notice that I switch master's base layer first and this branch still easily wins. If I do it in the opposite order, this branch wins even more easily. The video was recorded with the debugger set to throttle to 3G speed and to disable the cache in both windows.

I can provide an MP4 of this if anyone with skillz wants to edit it or whatever (e.g. speed it up).

@kring
Copy link
Member Author

kring commented Nov 10, 2016

And here's one showing a full reload at a zoomed-in view, same conditions. Not as drastic, and it's obvious here how non-optimal the loading still is, primarily because we currently don't know precisely where terrain tiles located until we load them, but it's still a nice improvement!
cesium-load-ordering-improvements-full-reload

@kring
Copy link
Member Author

kring commented Nov 10, 2016

But they were also previously sorted front-to-back for rendering, right?

Forgot to mention before that this isn't quite true. They're sorted by shader first, and by distance only for tiles that use the same shader. We need different shaders because different terrain tiles may have different numbers of imagery tiles attached to them.

@kring
Copy link
Member Author

kring commented Nov 10, 2016

Does the QuadtreePrimitive Sandcastle example need any updates?

Well, it doesn't seem to work even in master. Any objection to just removing the Sandcastle example, given that these days 3D Tiles is the "right" way to do what QuadtreePrimitive was meant to enable? I was thinking of removing it in the next round of refactoring anyway in order to simplify the code.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 10, 2016

Thanks, these animations are pure masterpieces; they will bring you much twitter fame.

Does the QuadtreePrimitive Sandcastle example need any updates?

Well, it doesn't seem to work even in master. Any objection to just removing the Sandcastle example, given that these days 3D Tiles is the "right" way to do what QuadtreePrimitive was meant to enable? I was thinking of removing it in the next round of refactoring anyway in order to simplify the code.

OK with me.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 10, 2016

Also, do you want to write a short (like 5-7 sentences) blog post that strings the animations together in a "Terrain/imagery engine is better" post?

@denverpierce
Copy link
Contributor

I ran this gist against this branch while monitoring ram, vram, cpu, and gpu usage. It probably wasn't an ideal way to test it, but I didn't notice any substantial difference in usage between this and master, except this branch seemed to causes occasional large GPU spikes (but no corresponding frame rate drops or anything noticeable).

@kring
Copy link
Member Author

kring commented Nov 11, 2016

Ok this is ready now (added another test).

Also, do you want to write a short (like 5-7 sentences) blog post that strings the animations together in a "Terrain/imagery engine is better" post?

Yes, but I want to fix that exception with missing tiles that was recently reported on the forum, and I want to make some more improvements to the terrain/imagery engine, and I can only let myself be distracted from other work for so long, so I think I'll pass on the blog for now.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 11, 2016

OK, do you mind if someone else wrote the blog post?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 11, 2016

@lilleyse if you have any comments, please open a PR or issue.

Thanks again @kring.

@pjcozzi pjcozzi merged commit f352a4f into master Nov 11, 2016
@pjcozzi pjcozzi deleted the depthFirst branch November 11, 2016 14:32
@kring
Copy link
Member Author

kring commented Nov 11, 2016

OK, do you mind if someone else wrote the blog post?

No that'd be great, thanks!

@jbo023 jbo023 mentioned this pull request Jan 20, 2017
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