Bugfix/bounding box bracket iteration#9
Conversation
Ikreb1
left a comment
Ikreb1
left a comment
There was a problem hiding this comment.
Thermo-nuclear code-quality review
The near-plane clipping rewrite is sound and the clip-space helpers (TransformPointToClip/AddNearPlaneIntersection/ProjectClipPoint/DistanceTo*Plane) are clean, correct, and the right fix for the vanishing-bracket bug — matrix order and the y-flipped viewport transform match the engine's row-vector convention and EveProjectBracket. m_bracketUpdateCallback correctly reuses the existing EveProjectBracket convention, and the EveRootTransform Blue mapping is valid (inherits via EveTransform).
Blocking concerns are structural, not in the math:
- Bounds-accumulation helpers triplicated across
EveTransform.cpp,EveEffectRoot2.cpp, and the bracket file — with inconsistent finiteness invariants (a latent NaN bug). Collapse to one shared accumulator (CcpMath::AxisAlignedBox/Utilities/BoundingBox.h). EveTransformwalksm_children4x with the same dynamic_cast + sphere-fallback; extract one helper.screenMarginis now a silent READWRITE no-op — resolve loudly.EveEffectRoot2.cppcrosses 1k lines as a direct result of (1).
Land the shared accumulator + single traversal first (deletes complexity, fixes the NaN issue by construction, keeps the file under 1k), then this is a clean approve. Inline comments below.
Verification: test in probe with a large object straddling the near plane and then engulfing the camera — that is the regression this targets and it needs a visual. Also grep the monolith for screenMargin and the new extendsOffscreen/coversViewport flags.
# Conflicts: # trinity/Eve/EveEffectRoot2.cpp # trinity/Eve/EvePlanet_Blue.cpp # trinity/Eve/EveTransform.h # trinity/Interior/Tr2InteriorPlaceable.cpp # trinity/Tr2ProjectBoundingBoxBracket.cpp # trinity/Tr2ProjectBoundingBoxBracket_Blue.cpp
There was a problem hiding this comment.
Pull request overview
This PR improves bracket projection robustness by replacing the previous screen-space clamping approach with a clip-space based bounding-box projection (including near-plane handling), and expands bounding-box support across several Eve/Interior objects via ITr2BoundingBox so they can participate in bracket-style tracking.
Changes:
- Reworked
Tr2ProjectBoundingBoxBracketprojection to compute a projected rect from clip-space points and added projection state outputs + an update callback. - Implemented/exposed
ITr2BoundingBoxon additional scene objects (EveTransform,EvePlanet,EveEffectRoot2) and surfaced the interface to Blue. - Adjusted
Tr2InteriorPlaceable::IsBoundingBoxReady()to account for modified bounds.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| trinity/Tr2ProjectBoundingBoxBracket.h | Adds projection state flags and a Blue callback; adds UpdateBracket() helper. |
| trinity/Tr2ProjectBoundingBoxBracket.cpp | Replaces projection logic with clip-space projection + near-plane intersection handling; centralizes bracket updates and invokes callback. |
| trinity/Tr2ProjectBoundingBoxBracket_Blue.cpp | Updates Blue exposure, deprecates screenMargin, exposes new projection state flags and callback. |
| trinity/Interior/Tr2InteriorPlaceable.cpp | Treats locally-modified bounding boxes as “ready” even if resources aren’t. |
| trinity/Eve/EveTransform.h | Makes EveTransform implement ITr2BoundingBox; fixes local-to-world transform to use m_worldTransform. |
| trinity/Eve/EveTransform.cpp | Implements local/world bounding box aggregation (including children) and readiness checks. |
| trinity/Eve/EveTransform_Blue.cpp | Exposes ITr2BoundingBox to Blue for EveTransform. |
| trinity/Eve/EveRootTransform_Blue.cpp | Exposes ITr2BoundingBox to Blue for EveRootTransform (via inheritance). |
| trinity/Eve/EvePlanet.h | Declares ITr2BoundingBox support for EvePlanet. |
| trinity/Eve/EvePlanet.cpp | Implements GetWorldBoundingBox()/IsBoundingBoxReady() for EvePlanet. |
| trinity/Eve/EvePlanet_Blue.cpp | Exposes ITr2BoundingBox to Blue for EvePlanet. |
| trinity/Eve/EveEffectRoot2.h | Declares ITr2BoundingBox support for EveEffectRoot2. |
| trinity/Eve/EveEffectRoot2.cpp | Implements world bounding box aggregation using root + child bounding spheres with validation. |
| trinity/Eve/EveEffectRoot2_Blue.cpp | Exposes ITr2BoundingBox to Blue for EveEffectRoot2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void GetLocalToWorldTransform( Matrix & transform ) const override | ||
| { | ||
| transform = IdentityMatrix(); | ||
| transform = m_worldTransform; |
There was a problem hiding this comment.
I need this changed but maybe it affects some other object I can't find a reason but maybe someone else knows
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cpp-linter Review
Used clang-format v20.1.2
Click here for the full clang-format patch
diff --git a/trinity/Eve/EveEffectRoot2.h b/trinity/Eve/EveEffectRoot2.h
index d9c6003..ff0cd82 100644
--- a/trinity/Eve/EveEffectRoot2.h
+++ b/trinity/Eve/EveEffectRoot2.h
@@ -95 +95 @@ public:
- bool GetWorldBoundingBox( Vector3& min, Vector3& max ) const override;
+ bool GetWorldBoundingBox( Vector3 & min, Vector3 & max ) const override;
Have any feedback or feature suggestions? Share it here.
|
|
||
| ///////////////////////////////////////////////////////////////////////////////////// | ||
| // ITr2BoundingBox | ||
| bool GetWorldBoundingBox( Vector3& min, Vector3& max ) const override; |
There was a problem hiding this comment.
clang-format suggestion
| bool GetWorldBoundingBox( Vector3& min, Vector3& max ) const override; | |
| bool GetWorldBoundingBox( Vector3 & min, Vector3 & max ) const override; |
Bounding-box bracket projection rework
https://fenriscreations.atlassian.net/browse/PLAT-11557
Summary
Rewrites
Tr2ProjectBoundingBoxBracketfrom screen-space clamping to clip-space projection with proper near-plane clipping, fixing brackets vanishing when the tracked object's bounds cross the near plane. Also:extendsOffscreen,coversViewport,containsCamera), reusing theEveProjectBracketconvention).ITr2BoundingBoxonEveTransform,EvePlanet,EveEffectRoot2(Blue-exposed, plusEveRootTransform) so more scene objects can be bracket-tracked.Tr2InteriorPlaceable::IsBoundingBoxReady()accounts for modified bounds.EveTransform::GetLocalToWorldTransformnow returnsm_worldTransforminstead of identity.Backwards incompatibility with EVE Online?
This might change the behavior in eve but I think not since my changes are behind flags and here is the only instance it is used:
eve/client/script/ui/login/charcreation_new/.../dollSelectionContainer.py: the character-creation doll-selection screen subclasses it withdefault_screenMargin = 10.0.