You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When cancelling a blend, CameraBlendStack was using a snapshot of the current state to keep track of the midpoint of the blend. This meant that if one of the two cameras were moving, the midpoint would become outdated.
This commit fixes the issue by introducing FrozenBlendSource an (internal) kind of nested blend source that will not advance the time (AdvanceBlend) but still update the child sources.
Commented all public classes, properties, and methods
Updated user documentation
Technical risk
Tech: Low Halo: Low
Comments to reviewers
I created a subclass of NestedBlendSource instead of modifying it directly because it allows scoping the concept of freezing the time of a blend source to the CameraBlendStack type.
I refactored a bit AdvanceBlend to make it less indented, only functional change should be CameraBlendStack.cs:345 (not ticking the time if source if FrozenBlendSource).
Fix hitch during blend cancellation by keeping underlying sources updating while freezing blend time for the cancelling midpoint.
Ensure cancellation accounts for updates to the outgoing camera state.
Add automated coverage to prevent regressions.
Update changelog/documentation as needed.
Requires further human verification:
Validate in the attached repro project that the hitch is gone in both Editor and Player.
Confirm behavior across the reported environments (Windows 11 and macOS) and affected Unity/package versions.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
Medium-sized behavioral change in blend progression logic plus a new test; requires careful reasoning about blend edge-cases (reversal, forced completion).
🏅 Score: 87
Solid targeted fix with test coverage, but there are a couple of edge-case behaviors (e.g., forced completion / negative delta) and test strength that should be re-validated.
AdvanceBlend skips incrementing TimeInBlend for FrozenBlendSource, including when deltaTime < 0 (the “force complete” path); confirm this cannot leave cancellation blends stuck or prevent expected fast-forward completion behavior in any call sites that rely on negative deltaTime.
// Advance the working blendif(AdvanceBlend(frame,deltaTime))frame.Source.ClearBlend();frame.UpdateCameraState(up,deltaTime);// local functionstaticboolAdvanceBlend(NestedBlendSourcesource,floatdeltaTime){varblend=source.Blend;if(blend.CamA==null)returnfalse;if(sourceis not FrozenBlendSource)blend.TimeInBlend+=(deltaTime>=0)?deltaTime:blend.Duration;if(blend.IsComplete){// No more blendblend.ClearBlend();returntrue;}if(blend.CamAisNestedBlendSourcebs)AdvanceBlend(bs,deltaTime);returnfalse;}
Backing out of a blend no longer forces snapshot = true; instead it wraps the chained blend in FrozenBlendSource. Confirm this preserves the historical “prevent pops” behavior in all reversal scenarios (especially with nested blends) and doesn’t reintroduce discontinuities.
// Special case: if backing out of a blend-in-progress// with the same blend in reverse, adjust the blend time// to cancel out the progress made in the opposite directionif(backingOutOfBlend){duration=duration*normalizedBlendPosition;// skip first part of blendnormalizedBlendPosition=1-normalizedBlendPosition;}// Chain to existing blendif(snapshot)camA=newSnapshotBlendSource(frame,frame.Blend.Duration-frame.Blend.TimeInBlend);else{varblendCopy=newCinemachineBlend();blendCopy.CopyFrom(frame.Blend);if(backingOutOfBlend)camA=newFrozenBlendSource(blendCopy);elsecamA=newNestedBlendSource(blendCopy);}
The new cancellation test moves both cameras equally along Z, which can make the assertion pass even if only one source is actually updating; consider moving only the outgoing camera (or moving them differently) and asserting multiple frames/expected interpolation to better detect stale midpoint state.
[Test]publicvoidTestBlendCancellationKeepsOutgoingCameraUpdating(){voidMoveCameras(){m_Cam1.Position+=Vector3.forward;m_Cam2.Position+=Vector3.forward;}Reset(1);// constant blend time of 1m_Cam1.Position=newVector3(0,0,0);m_Cam2.Position=newVector3(1,0,0);// Start with cam1, then blend towards cam2.ProcessFrame(m_Cam1,0.1f);MoveCameras();ProcessFrame(m_Cam2,0.5f);MoveCameras();Assume.That(m_BlendManager.IsBlending,Is.True);// Cancel/reverse the blend back to cam1 and advance a couple of frames.ProcessFrame(m_Cam1,0.1f);MoveCameras();ProcessFrame(m_Cam1,0.1f);Assume.That(m_BlendManager.IsBlending,Is.True);// Z coordinate should be the same for both cameras and blend resultAssert.AreEqual(m_Cam1.Position.z,m_BlendManager.CameraState.RawPosition.z,0.001f);}
Update review
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr
Ensure frozen blend sources still respect the "force complete" path when deltaTime < 0, otherwise cuts/forced-finishes can leave frozen nested blends lingering indefinitely. Keep the normal freeze behavior for deltaTime >= 0.
static bool AdvanceBlend(NestedBlendSource source, float deltaTime)
{
var blend = source.Blend;
if (blend.CamA == null)
return false;
- if (source is not FrozenBlendSource)+ // Freeze time during cancellation, but still allow forced completion (deltaTime < 0)+ if (source is not FrozenBlendSource || deltaTime < 0)
blend.TimeInBlend += (deltaTime >= 0) ? deltaTime : blend.Duration;
if (blend.IsComplete)
{
// No more blend
blend.ClearBlend();
return true;
}
if (blend.CamA is NestedBlendSource bs)
AdvanceBlend(bs, deltaTime);
return false;
}
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies an edge case where a FrozenBlendSource might never complete if a "force complete" signal (represented by deltaTime < 0) is ignored. This ensures consistency with how other blends handle forced completion.
Medium
General
Make the new test meaningful
This test currently moves both cameras equally and then asserts the blend output matches m_Cam1, which doesn't actually prove the outgoing (cancelled) side is still updating. Move only the cancelled-side camera during the cancellation blend and assert the blended Z changes accordingly, and use Assert.That instead of Assume.That to avoid silently inconclusive coverage.
[Test]
public void TestBlendCancellationKeepsOutgoingCameraUpdating()
{
- void MoveCameras() {- m_Cam1.Position += Vector3.forward;- m_Cam2.Position += Vector3.forward;- }-
Reset(1); // constant blend time of 1
m_Cam1.Position = new Vector3(0, 0, 0);
m_Cam2.Position = new Vector3(1, 0, 0);
// Start with cam1, then blend towards cam2.
ProcessFrame(m_Cam1, 0.1f);
- MoveCameras();
ProcessFrame(m_Cam2, 0.5f);
- MoveCameras();- Assume.That(m_BlendManager.IsBlending, Is.True);+ Assert.That(m_BlendManager.IsBlending, Is.True);- // Cancel/reverse the blend back to cam1 and advance a couple of frames.+ // Cancel/reverse the blend back to cam1.
ProcessFrame(m_Cam1, 0.1f);
- MoveCameras();+ Assert.That(m_BlendManager.IsBlending, Is.True);++ // Move only cam2: if the cancelled/outgoing source keeps updating, the result should react.+ m_Cam2.Position += Vector3.forward * 10f;
ProcessFrame(m_Cam1, 0.1f);
- Assume.That(m_BlendManager.IsBlending, Is.True);+ Assert.That(m_BlendManager.IsBlending, Is.True);- // Z coordinate should be the same for both cameras and blend result- Assert.AreEqual(m_Cam1.Position.z, m_BlendManager.CameraState.RawPosition.z, 0.001f);+ Assert.Greater(m_BlendManager.CameraState.RawPosition.z, m_Cam1.Position.z + 0.001f);
}
Suggestion importance[1-10]: 7
__
Why: The proposed test modification is superior because it isolates the camera being updated, providing a clearer verification that the outgoing camera's state is indeed being tracked. It also correctly recommends using Assert.That instead of Assume.That for critical state checks.
Medium
More suggestions
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr
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
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.
Purpose of this PR
Jira Issue Tracker
When cancelling a blend,
CameraBlendStackwas using a snapshot of the current state to keep track of the midpoint of the blend. This meant that if one of the two cameras were moving, the midpoint would become outdated.This commit fixes the issue by introducing FrozenBlendSource an (internal) kind of nested blend source that will not advance the time (AdvanceBlend) but still update the child sources.
Testing status
Only manually tested user repro project
Documentation status
Technical risk
Tech: Low Halo: Low
Comments to reviewers
I created a subclass of
NestedBlendSourceinstead of modifying it directly because it allows scoping the concept of freezing the time of a blend source to theCameraBlendStacktype.I refactored a bit
AdvanceBlendto make it less indented, only functional change should beCameraBlendStack.cs:345(not ticking the time if source if FrozenBlendSource).Package version