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.
1 participant
No description provided.