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.
| return boundsMax.x != boundsMin.x || boundsMax.y != boundsMin.y || boundsMax.z != boundsMin.z; | ||
| } | ||
|
|
||
| void IncludeBounds( const Vector3& boundsMin, const Vector3& boundsMax, Vector3& min, Vector3& max, bool& valid ) |
There was a problem hiding this comment.
[code-judo] Shared bounds accumulator. IncludeBounds here is byte-for-byte the same init-or-union as IncludeWorldBounds in EveEffectRoot2.cpp, and IncludeSphereBounds mirrors IncludeWorldSphereBounds there — three near-identical helper sets across this file, EveEffectRoot2.cpp, and the bracket file, with different validity rules. The canonical home already exists: Utilities/BoundingBox.h (BoundingBoxUpdate, BoundingBoxInitialize) and CcpMath::AxisAlignedBox with .Include() (used by CreateItemSetBoundingBoxes). The (min, max, bool& valid) triple is an AxisAlignedBox. One shared accumulator deletes the valid out-param plumbing, collapses 3 copies to 1, and fixes finiteness in one place. Please confirm the AxisAlignedBox API (Include(box)/Include(sphere)/empty-ctor).
| GetRenderables( renderables, nullptr ); | ||
| } | ||
|
|
||
| bool EveTransform::GetLocalBoundingBox( Vector3& min, Vector3& max ) |
There was a problem hiding this comment.
[dup] Same child walk 4x. GetLocalBoundingBox, GetWorldBoundingBox, IsBoundingBoxReady, and the existing GetBoundingSphere each re-implement dynamic_cast<ITr2BoundingBox*>(child) -> GetWorldBoundingBox, else GetBoundingSphere fallback. Extract one static bool GetChildWorldBounds(ITr2Renderable*, Vector3&, Vector3&) and route all three through it; IsBoundingBoxReady is just this loop early-exiting at the first contributor. (Don't collapse GetLocalBoundingBox to world x inverse — the per-child local transform is intentionally tighter — but the traversal helper still applies.) This same loop is a 5th copy in EveEffectRoot2.
| } | ||
| } | ||
|
|
||
| void IncludeSphereBounds( const Vector4& sphere, Vector3& min, Vector3& max, bool& valid ) |
There was a problem hiding this comment.
[latent bug] No finiteness guard. IncludeSphereBounds checks only w<=0 and IncludeBounds checks nothing, so a NaN/inf child box or sphere poisons the union (std::min/std::max propagate NaN) straight into the bracket projector. EveEffectRoot2's equivalents do guard (IsFinite/IsValidBoundingSphere), and the bracket re-guards everywhere — the invariant is enforced in 2 of 3 files. A single shared accumulator makes this unrepresentable instead of something to remember per call-site.
| m_screenMargin, | ||
| "Brackets are never projected outside the screen - this controls the margin from" | ||
| "\nthe edges of the screen.", | ||
| "Deprecated compatibility attribute. Bounding-box brackets are no longer" |
There was a problem hiding this comment.
[hidden behavior change] screenMargin is now a silent no-op but kept Be::READWRITE. Monolith Python that sets it will keep running and do nothing — a debugging trap. Either keep honoring it, drop the binding so callers break loudly and migrate, or at minimum make it Be::READ. The offscreen-clamping responsibility moved from C++ into the UI layer via extendsOffscreen/coversViewport — please grep the monolith to confirm those new flags are actually consumed before merge.
| m_extendsOffscreen = projectedBounds.extendsOffscreen; | ||
| m_coversViewport = projectedBounds.coversViewport; | ||
|
|
||
| float centerX = m_projectedX + m_projectedWidth * 0.5f; |
There was a problem hiding this comment.
[behavior change] Bounded brackets re-center. Old code centered bounded brackets (those with maxProjectedWidth/maxProjectedHeight) on the projected 3D center (useCenter3d); this always centers on the projected rect center. Defensible with correct clipping, but it is a visible change for max-clamped brackets — flag for whoever tunes them.
|
|
||
| extern float g_eveSpaceObjectResourceUnloadingTimeThreshold; | ||
|
|
||
| namespace |
There was a problem hiding this comment.
[file size + dup] This per-file helper toolkit is what pushes EveEffectRoot2.cpp from 996 -> 1099 lines (crosses the 1k smell threshold), and it duplicates EveTransform.cpp's helpers with stricter validity rules. Sharing one accumulator (see the EveTransform.cpp IncludeBounds comment) removes the toolkit and keeps this file under 1k.
| return false; | ||
| } | ||
|
|
||
| std::vector<ClipPoint> projectablePoints; |
There was a problem hiding this comment.
[perf] Per-frame heap alloc. std::vector<ClipPoint> with reserve(20) allocates per bracket per frame; the bound (8 corners + 12 edge intersections = 20) is known at compile time. Use std::array<ClipPoint, 20> + a count. Brackets update every frame and there can be many on screen.
| IncludeWorldBounds( rootMin, rootMax, min, max, valid ); | ||
| } | ||
|
|
||
| for( auto it = m_effectChildren.begin(); it != m_effectChildren.end(); ++it ) |
There was a problem hiding this comment.
[inconsistency] Children ignore ITr2BoundingBox here. This loop only calls GetBoundingSphere, never preferring a child's tighter GetWorldBoundingBox the way EveTransform::GetWorldBoundingBox does. If effect children never implement ITr2BoundingBox this is fine — but the two owners diverging in traversal strategy is the kind of drift that bites later. Worth a comment or unifying via the shared child-bounds helper.
| bool GetLocalBoundingBox( Vector3 &min, Vector3 &max ) override { return false; } | ||
| void GetLocalToWorldTransform( Matrix &transform ) const override { transform = IdentityMatrix(); } | ||
| bool GetLocalBoundingBox( Vector3 &min, Vector3 &max ) override; | ||
| void GetLocalToWorldTransform( Matrix &transform ) const override { transform = m_worldTransform; } |
There was a problem hiding this comment.
[verify] GetLocalToWorldTransform flips from IdentityMatrix() to m_worldTransform — clearly the correct fix, but confirm no picking/targeting caller relied on the old identity behavior.
# 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.
| m_projectedHeight = static_cast<float>( viewport.height ); | ||
| m_isProjectionValid = true; | ||
| m_containsCamera = true; | ||
| m_extendsOffscreen = true; |
| m_isProjectionValid = true; | ||
| UpdateBracket(); |
| 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
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) and an update callback (m_bracketUpdateCallback, 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. After this PR, doll cards near the viewport edge extend offscreen (buttons potentially unreachable) instead of clamping inside the margin.