Repository navigation
Use camera projection for LOD screen area - #3083
adrian-andrzjewski wants to merge 3 commits into
Conversation
|
@stephengold Could you please review this focused fix for #1784? It adds camera-projection-aware screen-area estimates for LOD while preserving the existing overload, with regression tests for perspective, parallel projection, and viewport scaling. The full jme3-core test suite passed locally (1,218 tests). |
| } else if (!projectionChanged && lastDistance > newDistance && lastLevel == 0) { | ||
| level = lastLevel; // we're already at the lowest setting and we just got closer to the model, no need to keep trying. | ||
| } else if (lastDistance < newDistance && lastLevel == numLevels - 1) { | ||
| } else if (!projectionChanged && lastDistance < newDistance && lastLevel == numLevels - 1) { |
There was a problem hiding this comment.
The cache key still only reflects center distance and camera projection; it doesn't include the world-bound size. If the geometry's scale changes while its bound center stays in place (common for a mesh centered at its origin), this branch reuses lastLevel and never recalculates the projected area, so the LOD can remain stale until the camera moves far enough. Could you include a bound-size/scale change in this invalidation check and add a regression case for resizing a stationary spatial? The PR summary specifically calls out scale invalidation, so it would be good to cover that path too.
There was a problem hiding this comment.
Thanks for catching this. Fixed in 9799c91: LodControl now includes the squared effective bound radius in its cache invalidation while retaining the distance tolerance. I added a regression for scaling a centered geometry at a fixed camera distance; it failed against the previous implementation and passes with this change. The focused AreaUtils/LodControl tests and the full jme3-core suite pass.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for putting together this focused fix for #1784. The camera-aware overload is a nice improvement: using the projection matrix and viewport dimensions makes the estimate useful across perspective, parallel, and sub-viewport cameras, while retaining the older overload preserves compatibility. I also appreciate the regression coverage for field of view and viewport scaling.
I reviewed the LOD cache behavior as well as the projection math and tests. One issue remains in LodControl: the cache invalidation tracks center distance and camera projection changes, but not changes to the spatial's world-bound size. A stationary, origin-centered geometry can be scaled without changing its center, which means the existing early-return path can keep the old LOD even though its screen area has changed. I left an inline note on that path; adding size/scale to the cache key and covering it with a regression test would make the stated scale invalidation reliable.
Thanks again for the careful compatibility work and test coverage. Once that cache case is handled, this should be in good shape.
|
@stephengold The cache invalidation feedback is addressed in 9799c91, and I replied to the inline review thread. The centered-scale regression failed before the fix and now passes; the full jme3-core suite passes as well. Please take another look when convenient. |
| float ratio = (FastMath.PI / (8f * atanNH)); | ||
| float newDistance = bv.distanceTo(vp.getCamera().getLocation()) / ratio; | ||
| float newDistance = bv.getCenter().distance(cam.getLocation()); | ||
| float boundRadiusSquared = getBoundRadiusSquared(bv); |
There was a problem hiding this comment.
The perspective estimate needs view-space depth here, rather than the Euclidean distance from the camera to the bound center. In a perspective projection, projected size depends on the center's depth along the camera's forward axis; moving a spatial sideways at the same depth should not make it appear smaller. With the current radial distance, off-axis geometry gets progressively coarser LODs even though its projected size is unchanged. Could this use the absolute camera-forward depth for perspective cameras (while keeping the parallel-projection behavior distance-independent)?
There was a problem hiding this comment.
Fixed in b8a0ccf. Perspective LOD now uses the absolute depth along the camera's forward axis; parallel projection uses no depth in the cache or area estimate. The new off-axis fixed-depth regression failed with radial distance and passes with the fix. Focused AreaUtils/LodControl tests and the full jme3-core suite pass.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for following up on the cache invalidation feedback and adding the fixed-distance scaling regression. The new bound-radius key closes the stale-LOD case I raised, and the tests give useful coverage for both the camera-aware area estimate and the resizing path.
I did find one remaining issue in the LOD integration: controlRender() passes the Euclidean distance from the camera to the bound center into a perspective projected-area calculation. Perspective size is determined by depth along the camera's viewing direction, not radial distance, so an object shifted sideways but kept at the same depth will be treated as smaller and may switch to a coarser LOD incorrectly. I left an inline comment on the distance calculation with the reasoning; using view-space depth for perspective projection should keep LOD stable across the viewport while preserving the intended behavior for parallel cameras.
Thanks again for addressing the earlier issue and for the focused tests. With that perspective-depth case handled, this will be in good shape.
|
@stephengold The off-axis perspective-depth feedback is addressed in b8a0ccf, following the earlier bound-size invalidation fix. I added fixed-depth regression coverage for both cases, reran the focused tests and full jme3-core suite, and updated the PR description. Please re-review when convenient. |
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for following up on both rounds of feedback and adding focused regressions for them. I’ve re-reviewed the updated diff: the perspective path now measures absolute depth along the camera’s forward axis, while parallel projection remains distance-independent, and the cache invalidates for changes in projected scale or world-bound size without losing its distance tolerance. The tests exercise the previously problematic fixed-depth sideways movement and fixed-distance scaling cases, alongside the projection and viewport area calculations.
I don’t see any remaining issues in the changes. The camera-aware overload also fits cleanly with the existing compatibility wrapper, and the reported focused and full core test runs are good coverage for this change. Thanks for the careful iterations—looks good to me!
|
adrian-andrzjewski I'm not available to review this code and have no connection with jaime-jmebot. Please stop tagging me on this PR. |
Summary
Fixes #1784
Validation
LodControlTestandAreaUtilsTestpassed.:jme3-core:testsuite passed.git diff --checkpassed.