test(bone-drag): cover entity tick + needUpdate between drags - #393
Merged
Conversation
📝 WalkthroughWalkthroughAdds two regression tests ensuring bone drag state does not accumulate across consecutive drags with auto-key disabled, covering an intermediate entity animation tick + skeleton transform update and a forced bone cache/transform recompute between drags. ChangesRegression Tests for Bone Drag State
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BoneDragRelease_test.cpp`:
- Around line 246-250: Replace the conditional check around the animation setup
with an explicit test assertion so the test fails if the animation is missing:
call ASSERT_TRUE(entity->hasAnimationState("TestAnim")) before retrieving the
state, then proceed to call entity->getAnimationState("TestAnim"),
state->setEnabled(true), and state->setTimePosition(0.0f) as before; this
ensures hasAnimationState, getAnimationState, setEnabled, and setTimePosition
are exercised only when the precondition is guaranteed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed827f0e-72ee-4752-a5f0-04ab41deafa2
📒 Files selected for processing (1)
src/BoneDragRelease_test.cpp
Comment on lines
+246
to
+250
| if (entity->hasAnimationState("TestAnim")) { | ||
| auto* state = entity->getAnimationState("TestAnim"); | ||
| state->setEnabled(true); | ||
| state->setTimePosition(0.0f); | ||
| } |
There was a problem hiding this comment.
Make the animation precondition explicit to avoid vacuous passes.
Using if (entity->hasAnimationState("TestAnim")) can let this regression pass without exercising the tick path if the fixture/asset changes. Prefer an assertion (ASSERT_TRUE(...)) before enabling/resetting the state so the test fails when its core precondition is missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BoneDragRelease_test.cpp` around lines 246 - 250, Replace the conditional
check around the animation setup with an explicit test assertion so the test
fails if the animation is missing: call
ASSERT_TRUE(entity->hasAnimationState("TestAnim")) before retrieving the state,
then proceed to call entity->getAnimationState("TestAnim"),
state->setEnabled(true), and state->setTimePosition(0.0f) as before; this
ensures hasAnimationState, getAnimationState, setEnabled, and setTimePosition
are exercised only when the precondition is guaranteed.
…drags Two new BoneDragRelease tests targeting the setUpdate-sequence gap: - EntityTickBetweenDragsDoesNotAccumulate: simulates an animation frame tick (entity->_updateAnimation + skeleton->_updateTransforms) between two drags. If the revert path doesn't fully restore TRS before clearing manualControlled, the tick re-applies the curve on top of leaked state. - NeedUpdateBetweenDragsDoesNotAccumulate: forces a derived-transform recompute via bone->needUpdate(true) + bone->_update(true,true) between drags so any cached state is flushed mid-sequence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
test/bone-drag-tick-coverage
branch
from
May 5, 2026 13:19
7488f07 to
243a930
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/BoneDragRelease_test.cpp (1)
246-250:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the animation precondition explicit to prevent vacuous passes.
At Line 246, the conditional can silently skip the core tick path if
"TestAnim"disappears from fixtures. Assert the precondition and proceed unconditionally with state setup.Suggested patch
- if (entity->hasAnimationState("TestAnim")) { - auto* state = entity->getAnimationState("TestAnim"); - state->setEnabled(true); - state->setTimePosition(0.0f); - } + ASSERT_TRUE(entity->hasAnimationState("TestAnim")); + auto* state = entity->getAnimationState("TestAnim"); + ASSERT_NE(state, nullptr); + state->setEnabled(true); + state->setTimePosition(0.0f);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BoneDragRelease_test.cpp` around lines 246 - 250, The test currently guards setup with entity->hasAnimationState("TestAnim") which can silently skip the tick path; replace that silent conditional with an explicit precondition assertion (e.g., assert or EXPECT_TRUE) that "TestAnim" exists, then unconditionally obtain the state via entity->getAnimationState("TestAnim") and call state->setEnabled(true) and state->setTimePosition(0.0f) so the test always performs the intended setup and fails loudly if the animation is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/BoneDragRelease_test.cpp`:
- Around line 246-250: The test currently guards setup with
entity->hasAnimationState("TestAnim") which can silently skip the tick path;
replace that silent conditional with an explicit precondition assertion (e.g.,
assert or EXPECT_TRUE) that "TestAnim" exists, then unconditionally obtain the
state via entity->getAnimationState("TestAnim") and call state->setEnabled(true)
and state->setTimePosition(0.0f) so the test always performs the intended setup
and fails loudly if the animation is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 450ae6bc-c55f-4beb-b0d0-ac38625f0fb9
📒 Files selected for processing (1)
src/BoneDragRelease_test.cpp
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two new
BoneDragReleasetests covering the setUpdate-sequence gap not exercised by the existing accumulation tests:EntityTickBetweenDragsDoesNotAccumulate— drivesentity->_updateAnimation()+skeleton->_updateTransforms()between two drags. If the revert path didn't fully restore TRS before clearingmanualControlled, the tick would re-apply the curve on top of leaked state and the second drag would start from a shifted pose.NeedUpdateBetweenDragsDoesNotAccumulate— forces a derived-transform recompute viabone->needUpdate(true)+bone->_update(true, true)between drags so any cached transform state is flushed mid-sequence.These complement the existing
AutoKeyOffTwoDragsDoNotAccumulate,YThenXDragsDoNotLeakYIntoX, andSetDerivedPositionDragRevertsCleanlytests by adding the "frame tick happens between drags" path that the user-visible bug report alludes to.Test-only change — no production code modifications.
Test plan
cmake --build build_local --target QtMeshEditorsucceeds./build_local/bin/UnitTests --gtest_filter="BoneDragReleaseTest.*"(auto-globbed)🤖 Generated with Claude Code
Summary by CodeRabbit