From 4d9e9446c79122182bc6783c345c5163623f7c2e Mon Sep 17 00:00:00 2001 From: Stephen L Peters Date: Thu, 17 Oct 2019 13:53:09 -0700 Subject: [PATCH] Fix two crashes from NativeAnimatedExamples (#3400) * merge in basic slider * Fix two issues: 1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag. * Change files * yarn format * Fix an issue with spring animations that did not start from 0, causing them to double count the start value. * respond to feedback. * yarn format --- ...10-11-13-21-33-ChainedAnimationsCrash.json | 8 ++ ...10-11-13-21-33-ChainedAnimationsCrash.json | 8 ++ ...10-11-13-21-33-ChainedAnimationsCrash.json | 8 ++ ...10-11-13-21-33-ChainedAnimationsCrash.json | 8 ++ vnext/ReactUWP/Base/UwpReactInstance.cpp | 2 + .../Modules/Animated/AdditionAnimatedNode.cpp | 4 +- .../Modules/Animated/DivisionAnimatedNode.cpp | 2 +- .../Modules/Animated/ModulusAnimatedNode.cpp | 2 +- .../Animated/MultiplicationAnimatedNode.cpp | 4 +- .../Modules/Animated/PropsAnimatedNode.cpp | 37 +++++++- .../Modules/Animated/PropsAnimatedNode.h | 3 + .../Animated/SpringAnimationDriver.cpp | 11 ++- .../Animated/SubtractionAnimatedNode.cpp | 4 +- vnext/ReactUWP/ReactUWP.vcxproj | 2 + vnext/ReactUWP/ReactUWP.vcxproj.filters | 8 +- vnext/ReactUWP/Views/SliderViewManager.cpp | 95 +++++++++++++++++++ vnext/ReactUWP/Views/SliderViewManager.h | 33 +++++++ 17 files changed, 225 insertions(+), 14 deletions(-) create mode 100644 change/react-native-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json create mode 100644 change/react-native-windows-extended-2019-10-11-13-21-33-ChainedAnimationsCrash.json create mode 100644 change/rnpm-plugin-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json create mode 100644 change/rnpm-plugin-wpf-2019-10-11-13-21-33-ChainedAnimationsCrash.json create mode 100644 vnext/ReactUWP/Views/SliderViewManager.cpp create mode 100644 vnext/ReactUWP/Views/SliderViewManager.h diff --git a/change/react-native-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json b/change/react-native-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json new file mode 100644 index 00000000000..28052467eea --- /dev/null +++ b/change/react-native-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json @@ -0,0 +1,8 @@ +{ + "type": "prerelease", + "comment": "Fix two issues: 1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag.", + "packageName": "react-native-windows", + "email": "stpete@microsoft.com", + "commit": "62049fdbd667fc71ae09b09f074446d8593d826d", + "date": "2019-10-11T20:21:32.900Z" +} \ No newline at end of file diff --git a/change/react-native-windows-extended-2019-10-11-13-21-33-ChainedAnimationsCrash.json b/change/react-native-windows-extended-2019-10-11-13-21-33-ChainedAnimationsCrash.json new file mode 100644 index 00000000000..bd621378ec7 --- /dev/null +++ b/change/react-native-windows-extended-2019-10-11-13-21-33-ChainedAnimationsCrash.json @@ -0,0 +1,8 @@ +{ + "type": "patch", + "comment": "Fix two issues: 1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag.", + "packageName": "react-native-windows-extended", + "email": "stpete@microsoft.com", + "commit": "62049fdbd667fc71ae09b09f074446d8593d826d", + "date": "2019-10-11T20:21:24.946Z" +} \ No newline at end of file diff --git a/change/rnpm-plugin-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json b/change/rnpm-plugin-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json new file mode 100644 index 00000000000..b52d3cb99fd --- /dev/null +++ b/change/rnpm-plugin-windows-2019-10-11-13-21-33-ChainedAnimationsCrash.json @@ -0,0 +1,8 @@ +{ + "type": "patch", + "comment": "Fix two issues: 1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag.", + "packageName": "rnpm-plugin-windows", + "email": "stpete@microsoft.com", + "commit": "62049fdbd667fc71ae09b09f074446d8593d826d", + "date": "2019-10-11T20:20:58.182Z" +} \ No newline at end of file diff --git a/change/rnpm-plugin-wpf-2019-10-11-13-21-33-ChainedAnimationsCrash.json b/change/rnpm-plugin-wpf-2019-10-11-13-21-33-ChainedAnimationsCrash.json new file mode 100644 index 00000000000..34fc30f8dbe --- /dev/null +++ b/change/rnpm-plugin-wpf-2019-10-11-13-21-33-ChainedAnimationsCrash.json @@ -0,0 +1,8 @@ +{ + "type": "none", + "comment": "Fix two issues: 1) you cannot animated 2 subchannels of the same property with different animations. to fix this we animated yet another property set for translation and scale owned by the props nodes and use one animation to animate all of the subchannels for the uiElement. 2) Reference parameter names which started with a multi digit number are unsupported so i added an n to the start of each name, which was previously just the node's tag.", + "packageName": "rnpm-plugin-wpf", + "email": "stpete@microsoft.com", + "commit": "62049fdbd667fc71ae09b09f074446d8593d826d", + "date": "2019-10-11T20:21:05.364Z" +} \ No newline at end of file diff --git a/vnext/ReactUWP/Base/UwpReactInstance.cpp b/vnext/ReactUWP/Base/UwpReactInstance.cpp index c5989279aa8..8cbf3e28ab7 100644 --- a/vnext/ReactUWP/Base/UwpReactInstance.cpp +++ b/vnext/ReactUWP/Base/UwpReactInstance.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -124,6 +125,7 @@ CreateUIManager( viewManagers.push_back(std::make_unique(instance)); viewManagers.push_back(std::make_unique(instance)); viewManagers.push_back(std::make_unique(instance)); + viewManagers.push_back(std::make_unique(instance)); viewManagers.push_back(std::make_unique(instance)); viewManagers.push_back(std::make_unique(instance)); viewManagers.push_back(std::make_unique(instance)); diff --git a/vnext/ReactUWP/Modules/Animated/AdditionAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/AdditionAnimatedNode.cpp index 82be88426ff..1dadfa3cdc8 100644 --- a/vnext/ReactUWP/Modules/Animated/AdditionAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/AdditionAnimatedNode.cpp @@ -12,7 +12,7 @@ AdditionAnimatedNode::AdditionAnimatedNode( int64_t tag, const folly::dynamic &config, const std::shared_ptr &manager) - : ValueAnimatedNode(tag, config, manager) { + : ValueAnimatedNode(tag, manager) { for (const auto &inputNode : config.find(s_inputName).dereference().second) { m_inputNodes.insert(static_cast(inputNode.asDouble())); } @@ -24,7 +24,7 @@ AdditionAnimatedNode::AdditionAnimatedNode( anim.Expression([nodes, manager, anim]() { winrt::hstring expr = L"0"; for (const auto tag : nodes) { - const auto identifier = std::to_wstring(tag); + const auto identifier = L"n" + std::to_wstring(tag); anim.SetReferenceParameter( identifier, manager->GetValueAnimatedNode(tag)->PropertySet()); expr = expr + L" + " + identifier + L"." + s_valueName + L" + " + diff --git a/vnext/ReactUWP/Modules/Animated/DivisionAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/DivisionAnimatedNode.cpp index 634fc3d4a25..441de5ce1f5 100644 --- a/vnext/ReactUWP/Modules/Animated/DivisionAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/DivisionAnimatedNode.cpp @@ -34,7 +34,7 @@ DivisionAnimatedNode::DivisionAnimatedNode( L"." + s_valueName + L" + " + s_baseName + L"." + s_offsetName + L")"; for (const auto tag : nodes) { - const auto identifier = std::to_wstring(tag); + const auto identifier = L"n" + std::to_wstring(tag); anim.SetReferenceParameter( identifier, manager->GetValueAnimatedNode(tag)->PropertySet()); expr = expr + L" / (" + identifier + L"." + s_valueName + L" " + diff --git a/vnext/ReactUWP/Modules/Animated/ModulusAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/ModulusAnimatedNode.cpp index 87118b88c2d..645f4579b37 100644 --- a/vnext/ReactUWP/Modules/Animated/ModulusAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/ModulusAnimatedNode.cpp @@ -12,7 +12,7 @@ ModulusAnimatedNode::ModulusAnimatedNode( int64_t tag, const folly::dynamic &config, const std::shared_ptr &manager) - : ValueAnimatedNode(tag, config, manager) { + : ValueAnimatedNode(tag, manager) { m_inputNodeTag = static_cast( config.find(s_inputName).dereference().second.asDouble()); m_modulus = static_cast( diff --git a/vnext/ReactUWP/Modules/Animated/MultiplicationAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/MultiplicationAnimatedNode.cpp index 5ea1eaeac3a..f28cf22e551 100644 --- a/vnext/ReactUWP/Modules/Animated/MultiplicationAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/MultiplicationAnimatedNode.cpp @@ -12,7 +12,7 @@ MultiplicationAnimatedNode::MultiplicationAnimatedNode( int64_t tag, const folly::dynamic &config, const std::shared_ptr &manager) - : ValueAnimatedNode(tag, config, manager) { + : ValueAnimatedNode(tag, manager) { for (const auto &inputNode : config.find(s_inputName).dereference().second) { m_inputNodes.insert(static_cast(inputNode.asDouble())); } @@ -24,7 +24,7 @@ MultiplicationAnimatedNode::MultiplicationAnimatedNode( anim.Expression([nodes, manager, anim]() { winrt::hstring expr = L"1"; for (const auto tag : nodes) { - auto identifier = std::to_wstring(tag); + auto identifier = L"n" + std::to_wstring(tag); anim.SetReferenceParameter( identifier, manager->GetValueAnimatedNode(tag)->PropertySet()); expr = expr + L" * (" + identifier + L"." + s_valueName + L" + " + diff --git a/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.cpp index 702d1e7be71..96d0e10e16b 100644 --- a/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.cpp @@ -21,6 +21,27 @@ PropsAnimatedNode::PropsAnimatedNode( m_propMapping.insert({entry.first.getString(), static_cast(entry.second.asDouble())}); } + + m_subchannelPropertySet = + winrt::Window::Current().Compositor().CreatePropertySet(); + m_subchannelPropertySet.InsertScalar(L"TranslationX", 0.0f); + m_subchannelPropertySet.InsertScalar(L"TranslationY", 0.0f); + m_subchannelPropertySet.InsertScalar(L"ScaleX", 1.0f); + m_subchannelPropertySet.InsertScalar(L"ScaleY", 1.0f); + + m_translationCombined = + winrt::Window::Current().Compositor().CreateExpressionAnimation( + L"Vector3(subchannels.TranslationX, subchannels.TranslationY, 0.0)"); + m_translationCombined.SetReferenceParameter( + L"subchannels", m_subchannelPropertySet); + m_translationCombined.Target(L"Translation"); + + m_scaleCombined = + winrt::Window::Current().Compositor().CreateExpressionAnimation( + L"Vector3(subchannels.ScaleX, subchannels.ScaleY, 1.0)"); + m_scaleCombined.SetReferenceParameter( + L"subchannels", m_subchannelPropertySet); + m_scaleCombined.Target(L"Scale"); } void PropsAnimatedNode::ConnectToView(int64_t viewTag) { @@ -89,7 +110,21 @@ void PropsAnimatedNode::StartAnimations() { if (const auto uiElement = GetUIElement()) { uiElement.RotationAxis(m_rotationAxis); for (const auto anim : m_expressionAnimations) { - uiElement.StartAnimation(anim.second); + if (anim.second.Target() == L"Translation.X") { + m_subchannelPropertySet.StartAnimation(L"TranslationX", anim.second); + uiElement.StartAnimation(m_translationCombined); + } else if (anim.second.Target() == L"Translation.Y") { + m_subchannelPropertySet.StartAnimation(L"TranslationY", anim.second); + uiElement.StartAnimation(m_translationCombined); + } else if (anim.second.Target() == L"Scale.X") { + m_subchannelPropertySet.StartAnimation(L"ScaleX", anim.second); + uiElement.StartAnimation(m_translationCombined); + } else if (anim.second.Target() == L"Scale.Y") { + m_subchannelPropertySet.StartAnimation(L"ScaleY", anim.second); + uiElement.StartAnimation(m_translationCombined); + } else { + uiElement.StartAnimation(anim.second); + } } if (m_needsCenterPointAnimation) { if (!m_centerPointAnimation) { diff --git a/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.h b/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.h index ed5afcbf869..38f1f5056be 100644 --- a/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.h +++ b/vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.h @@ -45,6 +45,9 @@ class PropsAnimatedNode : public AnimatedNode { nullptr}; winrt::Numerics::float3 m_rotationAxis{0, 0, 1}; bool m_needsCenterPointAnimation{false}; + winrt::CompositionPropertySet m_subchannelPropertySet{nullptr}; + winrt::CompositionAnimation m_translationCombined{nullptr}; + winrt::CompositionAnimation m_scaleCombined{nullptr}; static constexpr int64_t s_connectedViewTagUnset{-1}; }; diff --git a/vnext/ReactUWP/Modules/Animated/SpringAnimationDriver.cpp b/vnext/ReactUWP/Modules/Animated/SpringAnimationDriver.cpp index 0faa168a4fc..d446d401c16 100644 --- a/vnext/ReactUWP/Modules/Animated/SpringAnimationDriver.cpp +++ b/vnext/ReactUWP/Modules/Animated/SpringAnimationDriver.cpp @@ -56,7 +56,7 @@ SpringAnimationDriver::MakeAnimation(const folly::dynamic &config) { compositor.CreateLinearEasingFunction()); }(); - const auto startValue = GetAnimatedValue()->RawValue(); + const auto startValue = GetAnimatedValue()->Value(); std::vector keyFrames = [this, startValue]() { std::vector keyFrames; bool done = false; @@ -80,12 +80,15 @@ SpringAnimationDriver::MakeAnimation(const folly::dynamic &config) { animation.Duration(duration); auto normalizedProgress = 0.0f; - animation.InsertKeyFrame( - normalizedProgress, static_cast(startValue), easingFunction); + // We are animating the values offset property which should start at 0. + animation.InsertKeyFrame(normalizedProgress, 0.0f, easingFunction); for (const auto keyFrame : keyFrames) { normalizedProgress = std::min(normalizedProgress + 1.0f / keyFrames.size(), 1.0f); - animation.InsertKeyFrame(normalizedProgress, keyFrame, easingFunction); + animation.InsertKeyFrame( + normalizedProgress, + keyFrame - static_cast(startValue), + easingFunction); } if (m_iterations == -1) { diff --git a/vnext/ReactUWP/Modules/Animated/SubtractionAnimatedNode.cpp b/vnext/ReactUWP/Modules/Animated/SubtractionAnimatedNode.cpp index 10b40e0b524..156b8bed355 100644 --- a/vnext/ReactUWP/Modules/Animated/SubtractionAnimatedNode.cpp +++ b/vnext/ReactUWP/Modules/Animated/SubtractionAnimatedNode.cpp @@ -12,7 +12,7 @@ SubtractionAnimatedNode::SubtractionAnimatedNode( int64_t tag, const folly::dynamic &config, const std::shared_ptr &manager) - : ValueAnimatedNode(tag, config, manager) { + : ValueAnimatedNode(tag, manager) { for (const auto &inputNode : config.find(s_inputName).dereference().second) { if (m_firstInput == s_firstInputUnset) { m_firstInput = static_cast(inputNode.asDouble()); @@ -34,7 +34,7 @@ SubtractionAnimatedNode::SubtractionAnimatedNode( L"." + s_valueName + L" + " + s_baseName + L"." + s_offsetName + L")"; for (const auto tag : nodes) { - const auto identifier = std::to_wstring(tag); + const auto identifier = L"n" + std::to_wstring(tag); anim.SetReferenceParameter( identifier, manager->GetValueAnimatedNode(tag)->PropertySet()); expr = expr + L" - (" + identifier + L"." + s_valueName + L" + " + diff --git a/vnext/ReactUWP/ReactUWP.vcxproj b/vnext/ReactUWP/ReactUWP.vcxproj index d4bbb1178bc..9df13685d41 100644 --- a/vnext/ReactUWP/ReactUWP.vcxproj +++ b/vnext/ReactUWP/ReactUWP.vcxproj @@ -252,6 +252,7 @@ + @@ -350,6 +351,7 @@ + diff --git a/vnext/ReactUWP/ReactUWP.vcxproj.filters b/vnext/ReactUWP/ReactUWP.vcxproj.filters index 041693daed6..b27432f8e2d 100644 --- a/vnext/ReactUWP/ReactUWP.vcxproj.filters +++ b/vnext/ReactUWP/ReactUWP.vcxproj.filters @@ -298,6 +298,9 @@ Modules + + Views + @@ -633,9 +636,12 @@ Utils + + Views + Modules - + diff --git a/vnext/ReactUWP/Views/SliderViewManager.cpp b/vnext/ReactUWP/Views/SliderViewManager.cpp new file mode 100644 index 00000000000..519bdfeae80 --- /dev/null +++ b/vnext/ReactUWP/Views/SliderViewManager.cpp @@ -0,0 +1,95 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +#include "pch.h" + +#include +#include "SliderViewManager.h" + +#include + +#include + +#include + +namespace winrt { +using ToggleButton = Windows::UI::Xaml::Controls::Primitives::ToggleButton; +} + +namespace react { +namespace uwp { + +class SliderShadowNode : public ShadowNodeBase { + using Super = ShadowNodeBase; + + public: + SliderShadowNode() = default; + void createView() override; + void updateProperties(const folly::dynamic &&props) override; +}; + +void SliderShadowNode::createView() { + Super::createView(); +} + +void SliderShadowNode::updateProperties(const folly::dynamic &&props) { + m_updating = true; + Super::updateProperties(std::move(props)); + m_updating = false; +} + +SliderViewManager::SliderViewManager( + const std::shared_ptr &reactInstance) + : Super(reactInstance) {} + +const char *SliderViewManager::GetName() const { + return "RCTSlider"; +} + +folly::dynamic SliderViewManager::GetNativeProps() const { + auto props = Super::GetNativeProps(); + + props.update( + folly::dynamic::object("value", "integer")("disabled", "boolean")); + + return props; +} + +facebook::react::ShadowNode *SliderViewManager::createShadow() const { + return new SliderShadowNode(); +} + +XamlView SliderViewManager::CreateViewCore(int64_t tag) { + auto slider = winrt::Slider(); + return slider; +} + +void SliderViewManager::UpdateProperties( + ShadowNodeBase *nodeToUpdate, + const folly::dynamic &reactDiffMap) { + auto slider = nodeToUpdate->GetView().as(); + if (slider == nullptr) + return; + + for (const auto &pair : reactDiffMap.items()) { + const std::string &propertyName = pair.first.getString(); + const folly::dynamic &propertyValue = pair.second; + + if (propertyName == "disabled") { + if (propertyValue.isBool()) + slider.IsEnabled(!propertyValue.asBool()); + else if (pair.second.isNull()) + slider.ClearValue(winrt::Control::IsEnabledProperty()); + } else if (propertyName == "value") { + if (propertyValue.isNumber()) + slider.Value(propertyValue.asDouble()); + else if (pair.second.isNull()) + slider.Value(0); + } + } + + Super::UpdateProperties(nodeToUpdate, reactDiffMap); +} + +} // namespace uwp +} // namespace react diff --git a/vnext/ReactUWP/Views/SliderViewManager.h b/vnext/ReactUWP/Views/SliderViewManager.h new file mode 100644 index 00000000000..6f96e3c33d0 --- /dev/null +++ b/vnext/ReactUWP/Views/SliderViewManager.h @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +#pragma once + +#include + +namespace react { +namespace uwp { + +class SliderViewManager : public ControlViewManager { + using Super = ControlViewManager; + + public: + SliderViewManager(const std::shared_ptr &reactInstance); + + const char *GetName() const override; + folly::dynamic GetNativeProps() const override; + + facebook::react::ShadowNode *createShadow() const override; + + void UpdateProperties( + ShadowNodeBase *nodeToUpdate, + const folly::dynamic &reactDiffMap) override; + + protected: + XamlView CreateViewCore(int64_t tag) override; + + friend class SliderShadowNode; +}; + +} // namespace uwp +} // namespace react