Skip to content

Conversation

@StephenLPeters
Copy link
Contributor

@StephenLPeters StephenLPeters commented Jun 3, 2019

ReactNative asks the module to create 'nodes' which hold information which describes the animations. These nodes have a number of types: Value nodes which hold a value which changes over time because of animations. AnimationDriver nodes which associate with a Value node and change that value. Style nodes which associate a value with a string which identifies a property. And finally Props Nodes which associate a style with a particular XamlView.

The actual animations are implemented with composition animations. The value nodes contain a custom CompositionPropertySet which contains values which are animated using Composition::KeyFrameAnimation controlled by the animationdriver nodes. Finally these values are tied to the XamlView using Composition::ExpressionAnimation which are owned by the PropsNodes.

The unimplemented peices are:
1)Tracking Nodes
2)Spring Animations
3)Event Animations

Microsoft Reviewers: Open in CodeFlow

@StephenLPeters StephenLPeters requested a review from a team as a code owner June 3, 2019 16:56
@ghost ghost added the vnext label Jun 3, 2019
@StephenLPeters StephenLPeters self-assigned this Jun 3, 2019

modules.emplace_back(
NativeAnimatedModule::name,
[nativeAnimated = std::move(nativeAnimated)]() mutable { return std::make_unique<NativeAnimatedModule>(nativeAnimated); },
Copy link
Contributor

@licanhua licanhua Jun 3, 2019

Choose a reason for hiding this comment

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

make_unique [](start = 73, length = 11)

https://stackoverflow.com/questions/37884728/does-c11-unique-ptr-and-shared-ptr-able-to-convert-to-each-others-type
In short, you can easily and efficiently convert a std::unique_ptr to std::shared_ptr but you cannot convert std::shared_ptr to std::unique_ptr.
#Pending

@acoates-ms acoates-ms requested a review from PPatBoyd June 3, 2019 18:26
#include "pch.h"

namespace react {
namespace uwp {
Copy link
Contributor

@PPatBoyd PPatBoyd Jun 3, 2019

Choose a reason for hiding this comment

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

Everything but the animation driver should be shared (graph and nodes should be shared to win32) #Pending

void Offset(float offset);
void FlattenOffset();
void ExtractOffset();
winrt::CompositionPropertySet PropertySet() { return m_propertySet; };
Copy link
Contributor

Choose a reason for hiding this comment

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

since this class should likely be shareable with win32, is this the right place for a CompositionPropertySet?

{
using namespace ::winrt::Windows::UI::Xaml;
using namespace ::winrt::Windows::UI::Core;
using namespace Windows::Foundation;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot of usings in here; this seems like it could cause a lot of confusion about types. Is sharing the includes with presumably every other cppwinrt file useful? This impacts build times, etc. I wonder if the file should actually be removed instead of extended

@ghost ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 3, 2019
@ghost ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jun 20, 2019
@StephenLPeters StephenLPeters changed the title Opacity Animation Via NativeAnimatedModule NativeAnimedModule Jun 24, 2019
…eds to stop an animation prematurely.

Fix an issue where a callback in the NativeUiManager's BatchCompleted event added another callback to the list would crash.

Respond to some feedback
Copy link
Contributor

@kmelmon kmelmon left a comment

Choose a reason for hiding this comment

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

🕐

@ghost ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Jun 24, 2019
Copy link
Contributor

@kmelmon kmelmon left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@ahimberg ahimberg left a comment

Choose a reason for hiding this comment

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

I didn't mark all the asInt / getInt, and probably missed some for(auto : ) that should be & to avoid copies

@ahimberg ahimberg changed the title NativeAnimedModule NativeAnimatedModule Jun 26, 2019
…nager keep collections of unique pointers instead of shared pointers.
…ed with getters that return a reference to the underlying node.
@StephenLPeters StephenLPeters merged commit b38b0f8 into microsoft:master Jun 28, 2019
@StephenLPeters StephenLPeters deleted the NativeDriver branch June 28, 2019 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants