From 5bd5fcdd381acb65abfc5d1b50a2d342f220cd92 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Thu, 19 Nov 2020 14:50:25 -0800 Subject: [PATCH] LayoutAnimations: ensure that all mutations retain the correct "old" node Summary: The current implementation of LayoutAnimations assumed that the "previous/old" ShadowView passed into the diff mutation didn't matter except for purposes of diffing. As it turns out, iOS components could possibly use the "old" version of props, state, etc - so we should try to keep track of the current value in the tree as much as possible. This diff accomplishes that by keeping track of the "previous" view, which the AnimationDriver will update over time. This also allows us to simplify logic around conflicting animations. I'm also adding a few additional asserts to assist in debugging. This doesn't totally eliminate all asserts hit on iOS, yet, but it does reduce the number of times the asserts are hit in StubViewTree. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D25048644 fbshipit-source-id: d00aeece5af04624d8193063be453c7ce4a6e565 --- .../animations/LayoutAnimationDriver.cpp | 129 ++++-------------- .../animations/LayoutAnimationDriver.h | 4 - .../LayoutAnimationKeyFrameManager.cpp | 111 +++++++-------- .../LayoutAnimationKeyFrameManager.h | 8 +- .../react/renderer/mounting/StubViewTree.cpp | 3 + 5 files changed, 84 insertions(+), 171 deletions(-) diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp index 085cdff84b1..4c68ae3f45a 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.cpp @@ -29,96 +29,6 @@ namespace facebook { namespace react { -static double -getProgressFromValues(double start, double end, double currentValue) { - auto opacityMinmax = std::minmax({start, end}); - auto min = opacityMinmax.first; - auto max = opacityMinmax.second; - return ( - currentValue < min - ? 0 - : (currentValue > max ? 0 : ((max - currentValue) / (max - min)))); -} - -/** - * Given an animation and a ShadowView with properties set on it, detect how - * far through the animation the ShadowView has progressed. - * - * @param mutationsList - * @param now - */ -double LayoutAnimationDriver::getProgressThroughAnimation( - AnimationKeyFrame const &keyFrame, - LayoutAnimation const *layoutAnimation, - ShadowView const &animationStateView) const { - auto layoutAnimationConfig = layoutAnimation->layoutAnimationConfig; - auto const mutationConfig = - *(keyFrame.type == AnimationConfigurationType::Delete - ? layoutAnimationConfig.deleteConfig - : (keyFrame.type == AnimationConfigurationType::Create - ? layoutAnimationConfig.createConfig - : layoutAnimationConfig.updateConfig)); - - auto initialProps = keyFrame.viewStart.props; - auto finalProps = keyFrame.viewEnd.props; - - if (mutationConfig.animationProperty == AnimationProperty::Opacity) { - // Detect progress through opacity animation. - const auto &oldViewProps = - dynamic_cast(initialProps.get()); - const auto &newViewProps = - dynamic_cast(finalProps.get()); - const auto &animationStateViewProps = - dynamic_cast(animationStateView.props.get()); - if (oldViewProps != nullptr && newViewProps != nullptr && - animationStateViewProps != nullptr) { - return getProgressFromValues( - oldViewProps->opacity, - newViewProps->opacity, - animationStateViewProps->opacity); - } - } else if ( - mutationConfig.animationProperty != AnimationProperty::NotApplicable) { - // Detect progress through layout animation. - LayoutMetrics const &finalLayoutMetrics = keyFrame.viewEnd.layoutMetrics; - LayoutMetrics const &baselineLayoutMetrics = - keyFrame.viewStart.layoutMetrics; - LayoutMetrics const &animationStateLayoutMetrics = - animationStateView.layoutMetrics; - - if (baselineLayoutMetrics.frame.size.height != - finalLayoutMetrics.frame.size.height) { - return getProgressFromValues( - baselineLayoutMetrics.frame.size.height, - finalLayoutMetrics.frame.size.height, - animationStateLayoutMetrics.frame.size.height); - } - if (baselineLayoutMetrics.frame.size.width != - finalLayoutMetrics.frame.size.width) { - return getProgressFromValues( - baselineLayoutMetrics.frame.size.width, - finalLayoutMetrics.frame.size.width, - animationStateLayoutMetrics.frame.size.width); - } - if (baselineLayoutMetrics.frame.origin.x != - finalLayoutMetrics.frame.origin.x) { - return getProgressFromValues( - baselineLayoutMetrics.frame.origin.x, - finalLayoutMetrics.frame.origin.x, - animationStateLayoutMetrics.frame.origin.x); - } - if (baselineLayoutMetrics.frame.origin.y != - finalLayoutMetrics.frame.origin.y) { - return getProgressFromValues( - baselineLayoutMetrics.frame.origin.y, - finalLayoutMetrics.frame.origin.y, - animationStateLayoutMetrics.frame.origin.y); - } - } - - return 0; -} - void LayoutAnimationDriver::animationMutationsForFrame( SurfaceId surfaceId, ShadowViewMutation::List &mutationsList, @@ -132,7 +42,7 @@ void LayoutAnimationDriver::animationMutationsForFrame( } int incompleteAnimations = 0; - for (const auto &keyframe : animation.keyFrames) { + for (auto &keyframe : animation.keyFrames) { if (keyframe.type == AnimationConfigurationType::Noop) { continue; } @@ -164,10 +74,19 @@ void LayoutAnimationDriver::animationMutationsForFrame( // Create the mutation instruction auto updateMutation = ShadowViewMutation::UpdateMutation( - keyframe.parentView, baselineShadowView, mutatedShadowView, -1); + keyframe.parentView, keyframe.viewPrev, mutatedShadowView, -1); + + // All generated Update mutations must have an "old" and "new" + // ShadowView. Checking for nonzero tag doesn't guarantee that the views + // are valid/correct, just that something is there. + assert(updateMutation.oldChildShadowView.tag != 0); + assert(updateMutation.newChildShadowView.tag != 0); + mutationsList.push_back(updateMutation); PrintMutationInstruction("Animation Progress:", updateMutation); + keyframe.viewPrev = mutatedShadowView; + if (animationTimeProgressLinear < 1) { incompleteAnimations++; } @@ -200,23 +119,31 @@ void LayoutAnimationDriver::animationMutationsForFrame( // Copy so that if something else mutates the inflight animations, it // won't change this mutation after this point. - mutationsList.push_back( + auto mutation = ShadowViewMutation{finalMutationForKeyFrame.type, finalMutationForKeyFrame.parentShadowView, - finalMutationForKeyFrame.oldChildShadowView, + keyframe.viewPrev, finalMutationForKeyFrame.newChildShadowView, - finalMutationForKeyFrame.index}); + finalMutationForKeyFrame.index}; + assert(mutation.oldChildShadowView.tag != 0); + assert( + mutation.newChildShadowView.tag != 0 || + finalMutationForKeyFrame.type == ShadowViewMutation::Remove || + finalMutationForKeyFrame.type == ShadowViewMutation::Delete); + mutationsList.push_back(mutation); } else { // Issue a final UPDATE so that the final props object sent to the // mounting layer is the same as the one on the ShadowTree. This is // mostly to make the MountingCoordinator StubViewTree assertions // pass. - mutationsList.push_back( - ShadowViewMutation{ShadowViewMutation::Type::Update, - keyframe.parentView, - {}, - keyframe.viewEnd, - -1}); + auto mutation = ShadowViewMutation{ShadowViewMutation::Type::Update, + keyframe.parentView, + keyframe.viewPrev, + keyframe.viewEnd, + -1}; + assert(mutation.oldChildShadowView.tag != 0); + assert(mutation.newChildShadowView.tag != 0); + mutationsList.push_back(mutation); } } diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.h b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.h index 59033afca14..b0d0dabab85 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationDriver.h +++ b/ReactCommon/react/renderer/animations/LayoutAnimationDriver.h @@ -35,10 +35,6 @@ class LayoutAnimationDriver : public LayoutAnimationKeyFrameManager { SurfaceId surfaceId, ShadowViewMutation::List &mutationsList, uint64_t now) const override; - virtual double getProgressThroughAnimation( - AnimationKeyFrame const &keyFrame, - LayoutAnimation const *layoutAnimation, - ShadowView const &animationStateView) const override; }; } // namespace react diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp index 7ed615bef2e..248092f1ee0 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.cpp @@ -667,9 +667,7 @@ LayoutAnimationKeyFrameManager::getAndEraseConflictingAnimations( bool conflicting = animatedKeyFrame.tag == baselineShadowView.tag || ((mutation.type == ShadowViewMutation::Type::Delete || mutation.type == ShadowViewMutation::Type::Create) && - animatedKeyFrame.parentView.tag == baselineShadowView.tag) /* || - finalMutationTag == baselineShadowView.tag*/ - ; + animatedKeyFrame.parentView.tag == baselineShadowView.tag); // Conflicting animation detected: if we're mutating a tag under // animation, or deleting the parent of a tag under animation, or @@ -888,7 +886,8 @@ LayoutAnimationKeyFrameManager::pullTransaction( for (auto const &mutation : mutations) { ShadowView baselineShadowView = (mutation.type == ShadowViewMutation::Type::Delete || - mutation.type == ShadowViewMutation::Type::Remove + mutation.type == ShadowViewMutation::Type::Remove || + mutation.type == ShadowViewMutation::Type::Update ? mutation.oldChildShadowView : mutation.newChildShadowView); bool haveComponentDescriptor = @@ -949,7 +948,9 @@ LayoutAnimationKeyFrameManager::pullTransaction( mutation.newChildShadowView.tag, mutation.parentShadowView, movedIt->second.oldChildShadowView, - mutation.newChildShadowView}); + mutation.newChildShadowView, + movedIt->second.oldChildShadowView, + 0}); } } @@ -1017,6 +1018,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( parent, viewStart, viewFinal, + baselineShadowView, 0}; } else if (mutation.type == ShadowViewMutation::Type::Delete) { if (mutationConfig->animationProperty == @@ -1058,6 +1060,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( parent, viewStart, viewFinal, + baselineShadowView, 0}; } else if (mutation.type == ShadowViewMutation::Type::Update) { viewFinal = ShadowView(mutation.newChildShadowView); @@ -1069,6 +1072,7 @@ LayoutAnimationKeyFrameManager::pullTransaction( parent, viewStart, viewFinal, + baselineShadowView, 0}; } else { // This should just be "Remove" instructions that are not animated @@ -1092,8 +1096,9 @@ LayoutAnimationKeyFrameManager::pullTransaction( AnimationConfigurationType::Noop, tag, parent, - {}, - {}, + mutation.oldChildShadowView, + mutation.oldChildShadowView, + mutation.oldChildShadowView, 0}; } else { PrintMutationInstruction( @@ -1112,30 +1117,13 @@ LayoutAnimationKeyFrameManager::pullTransaction( // We've found a conflict. if (conflictingMutationBaselineShadowView.tag == tag) { - // What's the progress of this ongoing animation? - double conflictingAnimationProgress = - calculateAnimationProgress( - now, - *std::get<2>(conflictingKeyframeTuple), - std::get<1>(conflictingKeyframeTuple)) - .first; - - // Get a baseline ShadowView at the current progress of the - // inflight animation. TODO: handle multiple properties being - // animated separately? - auto interpolatedInflightShadowView = - createInterpolatedShadowView( - conflictingAnimationProgress, - conflictingKeyFrame.viewStart, - conflictingKeyFrame.viewEnd); - // Pick a Prop or layout property, depending on the current // animation configuration. Figure out how much progress we've // already made in the current animation, and start the animation // from this point. - keyFrame.viewStart = interpolatedInflightShadowView; - keyFrame.initialProgress = getProgressThroughAnimation( - keyFrame, &animation, interpolatedInflightShadowView); + keyFrame.viewStart = conflictingKeyFrame.viewPrev; + assert(keyFrame.viewStart.tag != 0); + keyFrame.initialProgress = 0; // We're guaranteed that a tag only has one animation associated // with it, so we can break here. If we support multiple @@ -1145,6 +1133,9 @@ LayoutAnimationKeyFrameManager::pullTransaction( } } + assert(keyFrame.viewStart.tag != 0); + assert(keyFrame.viewEnd.tag != 0); + assert(keyFrame.viewPrev.tag != 0); keyFramesToAnimate.push_back(keyFrame); } @@ -1185,36 +1176,32 @@ LayoutAnimationKeyFrameManager::pullTransaction( for (auto const &conflictingKeyframeTuple : conflictingAnimations) { auto &keyFrame = std::get<0>(conflictingKeyframeTuple); if (keyFrame.finalMutationForKeyFrame.hasValue()) { - auto &mutation = *keyFrame.finalMutationForKeyFrame; - if (mutation.type == ShadowViewMutation::Type::Update) { - auto mutationInstruction = ShadowViewMutation::UpdateMutation( - mutation.parentShadowView, - {}, - mutation.newChildShadowView, - mutation.index); - PrintMutationInstruction( - "Queueing up final mutation instruction - update:", - mutationInstruction); - finalConflictingMutations.push_back(std::move(mutationInstruction)); - } else { - PrintMutationInstruction( - "Queueing up final mutation instruction - non-update", - mutation); - finalConflictingMutations.push_back(mutation); - } + auto &finalMutation = *keyFrame.finalMutationForKeyFrame; + auto mutationInstruction = + ShadowViewMutation{finalMutation.type, + finalMutation.parentShadowView, + keyFrame.viewPrev, + finalMutation.newChildShadowView, + finalMutation.index}; + PrintMutationInstruction( + "Queueing up final mutation instruction - update:", + mutationInstruction); + assert(mutationInstruction.oldChildShadowView.tag != 0); + assert( + mutationInstruction.newChildShadowView.tag != 0 || + mutationInstruction.type == ShadowViewMutation::Delete || + mutationInstruction.type == ShadowViewMutation::Remove); + finalConflictingMutations.push_back(mutationInstruction); } else { // If there's no final mutation associated, create a mutation that // corresponds to the animation being 100% complete. This is important // for, for example, INSERT mutations being animated from opacity 0 // to 1. If the animation is interrupted we must force the View to be // at opacity 1. - auto mutatedShadowView = createInterpolatedShadowView( - 1, keyFrame.viewStart, keyFrame.viewEnd); auto generatedMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, - keyFrame.viewStart, - std::move(mutatedShadowView), - -1); + keyFrame.parentView, keyFrame.viewPrev, keyFrame.viewEnd, -1); + assert(generatedMutation.oldChildShadowView.tag != 0); + assert(generatedMutation.newChildShadowView.tag != 0); PrintMutationInstruction( "Queueing up final mutation instruction - synthetic", generatedMutation); @@ -1361,28 +1348,30 @@ LayoutAnimationKeyFrameManager::pullTransaction( for (auto const &conflictingKeyframeTuple : conflictingAnimations) { auto &keyFrame = std::get<0>(conflictingKeyframeTuple); if (keyFrame.finalMutationForKeyFrame.hasValue()) { + auto &finalMutation = (*keyFrame.finalMutationForKeyFrame); + auto mutation = ShadowViewMutation{finalMutation.type, + finalMutation.parentShadowView, + keyFrame.viewPrev, + finalMutation.newChildShadowView, + finalMutation.index}; PrintMutationInstruction( - "No Animation: Queueing final mutation instruction", - *keyFrame.finalMutationForKeyFrame); - finalMutationsForConflictingAnimations.push_back( - *keyFrame.finalMutationForKeyFrame); + "No Animation: Queueing up final conflicting mutation instruction", + mutation); + finalMutationsForConflictingAnimations.push_back(mutation); } else { // If there's no final mutation associated, create a mutation that // corresponds to the animation being 100% complete. This is important // for, for example, INSERT mutations being animated from opacity 0 // to 1. If the animation is interrupted we must force the View to be // at opacity 1. - auto mutatedShadowView = createInterpolatedShadowView( - 1, keyFrame.viewStart, keyFrame.viewEnd); auto generatedMutation = ShadowViewMutation::UpdateMutation( - keyFrame.parentView, - keyFrame.viewStart, - std::move(mutatedShadowView), - -1); + keyFrame.parentView, keyFrame.viewPrev, keyFrame.viewEnd, -1); + assert(generatedMutation.oldChildShadowView.tag != 0); + assert(generatedMutation.newChildShadowView.tag != 0); PrintMutationInstruction( "Queueing up final mutation instruction - synthetic", generatedMutation); - // Create the mutation instruction + // Create the mutation instructions finalMutationsForConflictingAnimations.push_back(generatedMutation); } } diff --git a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h index 5d69cec5688..6cb5176001a 100644 --- a/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h +++ b/ReactCommon/react/renderer/animations/LayoutAnimationKeyFrameManager.h @@ -103,6 +103,9 @@ struct AnimationKeyFrame { ShadowView viewStart; ShadowView viewEnd; + // ShadowView representing the previous frame of the animation. + ShadowView viewPrev; + // If an animation interrupts an existing one, the starting state may actually // be halfway through the intended transition. double initialProgress; @@ -252,11 +255,6 @@ class LayoutAnimationKeyFrameManager : public UIManagerAnimationDelegate, ShadowViewMutation::List &mutationsList, uint64_t now) const = 0; - virtual double getProgressThroughAnimation( - AnimationKeyFrame const &keyFrame, - LayoutAnimation const *layoutAnimation, - ShadowView const &animationStateView) const = 0; - SharedComponentDescriptorRegistry componentDescriptorRegistry_; mutable better::optional currentAnimation_{}; mutable std::mutex currentAnimationMutex_; diff --git a/ReactCommon/react/renderer/mounting/StubViewTree.cpp b/ReactCommon/react/renderer/mounting/StubViewTree.cpp index c2e3e3d62c5..4268b35e166 100644 --- a/ReactCommon/react/renderer/mounting/StubViewTree.cpp +++ b/ReactCommon/react/renderer/mounting/StubViewTree.cpp @@ -129,12 +129,15 @@ void StubViewTree::mutate(ShadowViewMutationList const &mutations) { STUB_VIEW_LOG({ LOG(ERROR) << "StubView: Update: " << mutation.newChildShadowView.tag; }); + STUB_VIEW_ASSERT(mutation.oldChildShadowView.tag != 0); + STUB_VIEW_ASSERT(mutation.newChildShadowView.tag != 0); STUB_VIEW_ASSERT(mutation.newChildShadowView.props); STUB_VIEW_ASSERT( mutation.newChildShadowView.tag == mutation.oldChildShadowView.tag); STUB_VIEW_ASSERT( registry.find(mutation.newChildShadowView.tag) != registry.end()); auto oldStubView = registry[mutation.newChildShadowView.tag]; + STUB_VIEW_ASSERT(oldStubView->tag != 0); STUB_VIEW_ASSERT( (ShadowView)(*oldStubView) == mutation.oldChildShadowView); oldStubView->update(mutation.newChildShadowView);