GeoPatch/GeoSphere refactoring

[fluffyfreak]

How it started:

I wanted to reduce the wasted space and padding in the classes GeoSphere and GeoPatch...

How's it's going:

... I reduced the wasted space and padding in the classes GeoSphere and GeoPatch, and then I went a bit crazy with refactoring things and just trying stuff out!!! 🤣

What else is here?:

I'd better just list things.

• fixes Terrain textures disappear #5806 by always render/updating the root GeoPatch
• experimental switch to using multiple smaller clipping spheres in the horizon test
• code deduplication in GeoPatch
• distance sorting of GeoPatches during rendering using gather/sort/dispatch
  • for future optimisation work like occlusion culling or depth-first rendering
• Terrain debug tab in PerfInfo, feedback and icon needed
• renamed Frustum::TestPoint to Frustum::TestSphere because that's what it does
• added slow zoom using L/R shift keys in Object Viewer
• added Drawables::Label3D for showing GeoPatch Face ID
  • not yet toggleable but under define

Performance Improvement?

Absolutely no performance improvement that I can tell but then this was all done in areas that medium to high-end PCs just don't struggle with. I haven't tested it on the RPi5 very much and not at all since the profiling has been working again.

Tested on lower end machines and there is a definite performance improvement on the Raspberry Pi 5 and low-end x86/64 machines! Particularly with integrated graphics.

[bszlrd]

There should be a planet icon in the set already, but I can draw another one if needed/wanted

[sturnclaw]

I'm going to have to break out NSight and dig into some of this code to get an accurate read on the GPU performance implications - currently productively procrastinating by working on other areas of the render code so it will likely be a day or more before I start going through the code.

[sturnclaw]

Going to add comments here incrementally as I have time today.

[sturnclaw]

Ran it through NSight - thanks for making the distance sort a configurable option!

I'm seeing similar results to what you described; terrain rendering takes 0.5-0.6ms regardless of option, and we're strongly limited by register allocation and basic floating-point unit throughput (no surprise there). Assuming we have early-Z fragment discard working properly (no way to check as far as I can tell) doing a distance sort could allow ~5-10% of fragments to be skipped in the best case.

On a 2070 Super, that is a negligible difference in frame timing. On a laptop GPU or handheld device, that could be a few tenths of a millisecond (or more). I definitely think its an optimization worth keeping.

Completely spitballing here, but have you considered frustum-OBB culling for the horizon culling method instead? I'd expect you'd get a significantly more accurate cull (assuming maximum-feature-height per patch is correctly implemented) at the expense of more CPU time.

Frustum-OBB can potentially be decomposed to 5x AABB-Plane tests, which can be quite cheap (note: the link computes intersection, but could likely be amended to compute is-on-front). The main cost would be transforming each plane of the frustum to the orientation of the OBB (as well as the cost of storing the OBB).

[fluffyfreak]

pioneer-profile-20241122_140455.zip
Makes an exciting amount of difference on the RPi5!
Mars start, medium city and planet, 720p. Equivalent of 31fps to 39fps

[fluffyfreak]

Completely spitballing here, but have you considered frustum-OBB culling for the horizon culling method instead? I'd expect you'd get a significantly more accurate cull (assuming maximum-feature-height per patch is correctly implemented) at the expense of more CPU time.

Honestly I never considered doing 90% of this PR 🤣 I just got distracted whilst doing the padding and size redunction!
It's a rare win for my ADHD!

So I'd not looked into more optimal culling designs yet. Also when I looked at the wireframe view it really didn't seem like there's a lot of easy wins left with round planets 🤷

Still might worth looking into something like Improved Frustum Culling eventually. However I don't want this PR to grow and grow.

[sturnclaw]

Honestly I never considered doing 90% of this PR 🤣 I just got distracted whilst doing the padding and size redunction!
It's a rare win for my ADHD!

Heh, I know the feeling. The distance-sort change is definitely a win and there are a few other nice improvements in the PR as well.

So I'd not looked into more optimal culling designs yet. Also when I looked at the wireframe view it really didn't seem like there's a lot of easy wins left with round planets 🤷

It's probably not necessary at this point. I don't imagine we'd see a significant performance improvement, and I suspect half of the culling problem is that the horizon culling is culling based on the nominal radius of the SystemBody rather than the minimum feature height of the visible horizon. That's a non-trivial thing to compute however, as it varies based on view direction.

---

I'll continue reviewing this as I have time - going to bounce around to a few other open PRs. The most recent commits you pushed look good, though if we adopt distance sort for terrain I'm not quite sure we need the terrain debug info tab?

[fluffyfreak]

though if we adopt distance sort for terrain I'm not quite sure we need the terrain debug info tab?

I have other uses planned for it, like freezing updates, toggling new rendering stuff I want to try out, pulling out some stats about updates, and generation latency between request and receive, how many patches we have at each depth level, etc. Things that don't seem to fit elsewhere but I hope will teach me more about how the terrain performs so I can think about refactoring it.

[fluffyfreak]

I had a chance to run this on an old laptop with integrated graphics last night, it makes a big difference to the framerate on low-end hardware! I'd have to check again for get consistent framerate figures but it was ~20-25% improvement.

[sturnclaw]

Nice work! I'll give this a closer review soon.

[fluffyfreak]

@impaktor I know the rendering stuff isn't your favourite area, but would you mind reviewing this with an eye to helping me cleanup the commit history as I'd really like to get this in sometime soon?

[impaktor]

I don't really have a good workflow / tools to go through githistory of a line (there's of course a mode for that in Emacs, but I've not tried it), so just skimming through these greek hieroglyphs and caching them to gray matter, looks like there would be some squash / fixup (if not familiar with rebase: "fixup" will squash the commit with the one above it):

pick Label3DWrapper to wrap Label3D from Scenegraph for use elsewhere 2f9d3e0
fixup Remove Scenegraph::Label3D references aadf6dc

pick Rework GeoPatch to reduce size 2871b33
fixup Don't include "Core/Log.h" 7b64f71

pick Experiment sub-centroid patch culling f85f62c
fixup Possibly more optimal ordering of sample points to check ed492dd

@fluffyfreak Do you want me to do rebase and force push back to this branch?

[sturnclaw]

My apologies for the delay - things have been quite busy on my end. I most likely won't be able to give this a full review until early next week, but cleaning up the git history will help make it easier to review 😄

[fluffyfreak]

@impaktor yeah that'd be awesome if you could. My work has exploded again. Always like this at xmas so I'm struggling with time and that'd be a big help 👍

@sturnclaw totally understand, life gets busy and time gets short all at once at this time of year

[impaktor]

I've gone throuhg it. I expect the commits of a branch to be on the top of git log, i.e. to be on the tip of the branch, but here, they were buried far down with plenty of other commits from master ontop of it. I assume this is due to a merge commit. If a branch needs updating, rebase is the way to go, as it will just pull in master and move all commits of the branch to the tip.

So I did a hard reset of this branch to master, then cherry-picked each commit manually to bring them to the front, then rebased 3 commits.

Additional rebase could be done?

I did get some strange conflicts when trying to rebase the "Core/Log.h" (so I aborted that one)

pick 6ff8d36 * Rework GeoPatch to reduce size
fixup 60a0059 * Don't include "Core/Log.h"

And I suspect these two could be squashed:

pick bb13e4b * WIP PiGui::PerfInfo::DrawTerrainDebug
fixup f12d1da * Fixed perf icon & tab rendering for MSVC

But I leave that as an exercise to @sturnclaw, if he cares about it enough. And any additional rebase can easily be made now that the commits are at the tip of the branch.

Review

What ever requested changes, still apply, I assume, just the commit hash is new so I assume github has marked them as "outdated"

Re-pulling your branch

Since I've now borked your branch, I think you could do something like (assuming your github repo is origin):

git fetch --all -p
git checkout reduce-geopatch-size
git reset --hard origin/reduce-geopatch-size 

I have a copy of your original branch if you want to revert (plus it's somewhere in reflog as well)

[fluffyfreak]

That is fantastic! Thanks so much @impaktor !
I believe that I had already addressed the requested changes so honestly it's good that it has changed 👍