Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix two crashes from NativeAnimatedExamples #3400

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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"
}
Original file line number Diff line number Diff line change
@@ -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"
}
Original file line number Diff line number Diff line change
@@ -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"
}
Original file line number Diff line number Diff line change
@@ -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"
}
2 changes: 2 additions & 0 deletions vnext/ReactUWP/Base/UwpReactInstance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <Views/RootViewManager.h>
#include <Views/ScrollContentViewManager.h>
#include <Views/ScrollViewManager.h>
#include <Views/SliderViewManager.h>
#include <Views/SwitchViewManager.h>
#include <Views/TextInputViewManager.h>
#include <Views/TextViewManager.h>
Expand Down Expand Up @@ -124,6 +125,7 @@ CreateUIManager(
viewManagers.push_back(std::make_unique<RawTextViewManager>(instance));
viewManagers.push_back(std::make_unique<RootViewManager>(instance));
viewManagers.push_back(std::make_unique<ScrollContentViewManager>(instance));
viewManagers.push_back(std::make_unique<SliderViewManager>(instance));
viewManagers.push_back(std::make_unique<ScrollViewManager>(instance));
viewManagers.push_back(std::make_unique<SwitchViewManager>(instance));
viewManagers.push_back(std::make_unique<TextViewManager>(instance));
Expand Down
4 changes: 2 additions & 2 deletions vnext/ReactUWP/Modules/Animated/AdditionAnimatedNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ AdditionAnimatedNode::AdditionAnimatedNode(
int64_t tag,
const folly::dynamic &config,
const std::shared_ptr<NativeAnimatedNodeManager> &manager)
: ValueAnimatedNode(tag, config, manager) {
: ValueAnimatedNode(tag, manager) {
for (const auto &inputNode : config.find(s_inputName).dereference().second) {
m_inputNodes.insert(static_cast<int64_t>(inputNode.asDouble()));
}
Expand All @@ -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" + " +
Expand Down
2 changes: 1 addition & 1 deletion vnext/ReactUWP/Modules/Animated/DivisionAnimatedNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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" " +
Expand Down
2 changes: 1 addition & 1 deletion vnext/ReactUWP/Modules/Animated/ModulusAnimatedNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ ModulusAnimatedNode::ModulusAnimatedNode(
int64_t tag,
const folly::dynamic &config,
const std::shared_ptr<NativeAnimatedNodeManager> &manager)
: ValueAnimatedNode(tag, config, manager) {
: ValueAnimatedNode(tag, manager) {
m_inputNodeTag = static_cast<int64_t>(
config.find(s_inputName).dereference().second.asDouble());
m_modulus = static_cast<int64_t>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ MultiplicationAnimatedNode::MultiplicationAnimatedNode(
int64_t tag,
const folly::dynamic &config,
const std::shared_ptr<NativeAnimatedNodeManager> &manager)
: ValueAnimatedNode(tag, config, manager) {
: ValueAnimatedNode(tag, manager) {
for (const auto &inputNode : config.find(s_inputName).dereference().second) {
m_inputNodes.insert(static_cast<int64_t>(inputNode.asDouble()));
}
Expand All @@ -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" + " +
Expand Down
37 changes: 36 additions & 1 deletion vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,27 @@ PropsAnimatedNode::PropsAnimatedNode(
m_propMapping.insert({entry.first.getString(),
static_cast<int64_t>(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) {
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions vnext/ReactUWP/Modules/Animated/PropsAnimatedNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -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};
};
Expand Down
11 changes: 7 additions & 4 deletions vnext/ReactUWP/Modules/Animated/SpringAnimationDriver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ SpringAnimationDriver::MakeAnimation(const folly::dynamic &config) {
compositor.CreateLinearEasingFunction());
}();

const auto startValue = GetAnimatedValue()->RawValue();
const auto startValue = GetAnimatedValue()->Value();
std::vector<float> keyFrames = [this, startValue]() {
std::vector<float> keyFrames;
bool done = false;
Expand All @@ -80,12 +80,15 @@ SpringAnimationDriver::MakeAnimation(const folly::dynamic &config) {
animation.Duration(duration);

auto normalizedProgress = 0.0f;
animation.InsertKeyFrame(
normalizedProgress, static_cast<float>(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<float>(startValue),
easingFunction);
}

if (m_iterations == -1) {
Expand Down
4 changes: 2 additions & 2 deletions vnext/ReactUWP/Modules/Animated/SubtractionAnimatedNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ SubtractionAnimatedNode::SubtractionAnimatedNode(
int64_t tag,
const folly::dynamic &config,
const std::shared_ptr<NativeAnimatedNodeManager> &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<int64_t>(inputNode.asDouble());
Expand All @@ -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" + " +
Expand Down
2 changes: 2 additions & 0 deletions vnext/ReactUWP/ReactUWP.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@
<ClInclude Include="Views\ScrollContentViewManager.h" />
<ClInclude Include="Views\ScrollViewManager.h" />
<ClInclude Include="Views\SwitchViewManager.h" />
<ClInclude Include="Views\SliderViewManager.h" />
<ClInclude Include="Views\TextInputViewManager.h" />
<ClInclude Include="Views\TextViewManager.h" />
<ClInclude Include="Views\TouchEventHandler.h" />
Expand Down Expand Up @@ -359,6 +360,7 @@
<ClCompile Include="Views\RootViewManager.cpp" />
<ClCompile Include="Views\ScrollContentViewManager.cpp" />
<ClCompile Include="Views\ScrollViewManager.cpp" />
<ClCompile Include="Views\SliderViewManager.cpp" />
<ClCompile Include="Views\ShadowNodeBase.cpp" />
<ClCompile Include="Views\SwitchViewManager.cpp" />
<ClCompile Include="Views\TextInputViewManager.cpp" />
Expand Down
8 changes: 7 additions & 1 deletion vnext/ReactUWP/ReactUWP.vcxproj.filters
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,9 @@
<ClCompile Include="Modules\StatusBarModule.cpp">
<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="Views\SliderViewManager.cpp">
<Filter>Views</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<ClInclude Include="Modules\DeviceInfoModule.h">
Expand Down Expand Up @@ -633,9 +636,12 @@
<ClInclude Include="Utils\CppWinrtLessExceptions.h">
<Filter>Utils</Filter>
</ClInclude>
<ClInclude Include="Views\SliderViewManager.h">
<Filter>Views</Filter>
</ClInclude>
<ClInclude Include="Modules\StatusBarModule.h">
<Filter>Modules</Filter>
</ClInclude>
</ClCompile>
</ItemGroup>
<ItemGroup>
<None Include="EndPoints\dll\react-native-uwp.arm.def">
Expand Down
95 changes: 95 additions & 0 deletions vnext/ReactUWP/Views/SliderViewManager.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#include "pch.h"

#include <Views/ShadowNodeBase.h>
#include "SliderViewManager.h"
Copy link
Contributor

@licanhua licanhua Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#include <Views/SliderViewManager.h> ? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not how any of the other view managers include their header


In reply to: 335125441 [](ancestors = 335125441)


#include <Utils/ValueUtils.h>

#include <IReactInstance.h>

#include <winrt/Windows.UI.Xaml.Controls.Primitives.h>

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;
Copy link
Contributor

@licanhua licanhua Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_updating [](start = 2, length = 10)

why do you need m_updating flag? #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the shadow nodes do this, i couldn't tell you why


In reply to: 335127109 [](ancestors = 335127109)

Super::updateProperties(std::move(props));
m_updating = false;
}

SliderViewManager::SliderViewManager(
const std::shared_ptr<IReactInstance> &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<winrt::Slider>();
Copy link
Contributor

@licanhua licanhua Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as [](start = 40, length = 2)

I guess you means try_as. otherwise as would crash if it's null #Resolved

if (slider == nullptr)
return;
Copy link
Contributor

@licanhua licanhua Oct 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to do this check. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do, as and try_as return null if the QI fails


In reply to: 335128424 [](ancestors = 335128424)


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
33 changes: 33 additions & 0 deletions vnext/ReactUWP/Views/SliderViewManager.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#pragma once

#include <Views/ControlViewManager.h>

namespace react {
namespace uwp {

class SliderViewManager : public ControlViewManager {
using Super = ControlViewManager;

public:
SliderViewManager(const std::shared_ptr<IReactInstance> &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