Repository navigation
Refresh compound collision debug shapes (#1346) - #3085
adrian-andrzjewski wants to merge 2 commits into
Conversation
| */ | ||
| protected final void cacheCompoundChildren(CollisionShape shape) { | ||
| if (shape instanceof CompoundCollisionShape) { | ||
| compoundChildrenSnapshot = new ArrayList<>( |
There was a problem hiding this comment.
One edge case worth covering here: this is only a shallow snapshot of the child list, and ChildCollisionShape exposes mutable location, rotation, and shape fields. If a caller edits one of those fields in place, the snapshot and current list still contain the same child object, so List.equals() reports no change and the debug geometry keeps the old transform/mesh. Could the comparison snapshot those child values (or otherwise detect in-place edits) as well as additions, removals, and replacements?
There was a problem hiding this comment.
Thanks for catching this. The snapshot now stores each child shape identity and copies of its location and rotation, and the control rebuilds when any of those values change. I added regressions for in-place location, rotation, and shape edits; :jme3-jbullet:check passes with all five debug-control tests passing.
There was a problem hiding this comment.
Thanks for the follow-up — the snapshot now captures the child shape identity and copies of its location and rotation, and the added tests exercise each of those in-place edits. I’ve checked the updated change and the shared control refreshes the geometry when any captured child property differs. This addresses my concern; I’m happy with the fix.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for addressing the stale compound debug-shape issue across rigid bodies, ghosts, and characters, and for adding headless regression coverage. The cached child list is a good way to avoid rebuilding the debug node every update when the shape is unchanged, while still catching same-count replacements.
I checked the shared control logic against CompoundCollisionShape and its child representation. The current snapshot detects structural list changes, but it does not detect edits made directly to an existing child’s mutable location, rotation, or shape fields: because the snapshot holds the same ChildCollisionShape instances, such in-place edits compare equal and the displayed geometry remains stale. I left an inline comment on the snapshot logic asking that these changes be detected too (or that the supported mutation contract be clarified and enforced).
The tests cover removal, replacement, and reuse well. Once the mutable-child case is accounted for, this should provide reliable refresh behavior for the compound debug shapes. Thanks for the thoughtful fix!
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Hi, thanks for following through on the compound-child update and adding coverage for the in-place edits. The snapshot now captures each child’s shape identity plus copies of its location and rotation, so the shared refresh check detects both structural changes and mutations that would otherwise leave the debug mesh stale. The rigid-body tests cover those cases as well as unchanged-geometry reuse, and the same check is wired into rigid-body, ghost, and character controls.
This addresses my earlier concern, and the implementation preserves the useful fast path when the compound is unchanged. The reported :jme3-jbullet:check run is encouraging; I don’t see further issues in the updated diff. Thanks for the solid fix!
Summary
Validation
Closes #1346