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

AnimatedShape implementation #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lelandrichardson
Copy link
Contributor

to: @vjeux @browniefed

This is an implementation of AnimatedShape, which is a proposed solution to the problem of us not having the ability to create animated values of arbitrary shape.

One such example of this is animated regions or coordinates for maps. Similar to how Animated.ValueXY is an animated value class for a shape of { x: number, y: number }, it is conceivable we would want to make a class that had a shape of { latitude: number, longitude: number, latitudeDelta: number, longitudeDelta: number }, but it is not currently possible because we do not export the proper classes to export from.

The proposal is for an Animated.Shape export that allows us to implement this like so:

var Animated = require('animated');

var region = new Animated.Shape({
  latitude: 1,
  longitude: 2,
  latitudeDelta: 3,
  longitudeDelta: 4,
});

Additionally, if I wanted to implement some custom prototype methods on top of a shape:

var Animated = require('animated');

class AnimatedRegion extends Animated.Shape {
  constructor(region) {
    super(region);
  }
  customMethod() { ... }
}

Notes:

This PR is easier to look at if you just pay attention to the second commit. The first commit is some reorganization in order to allow the .spring() and .timing() instance methods, I basically had to move some of the exported methods into their own files so we could reference them. Not really important to this PR.

TODO

I'd like to implement an additional .interpolate() method that could be used like this:

var interpolated = region.interpolate({
  latitude: {
    inputRange: [...],
    outputRange: [...],
  },
  longitude: {
    inputRange: [...],
    outputRange: [...],
  },
  ...
});

@sahrens
Copy link
Collaborator

sahrens commented Feb 29, 2016

This is cool, but not certain it's worth it - ValueXY is just shorthand since it's really common, but I'm not sure how common arbitrary shapes are...does this actually enable anything that's currently not possible, or just make it a little more convenient to code up?

If you built this in a general way and used it to implement ValueXY I could see this being a net positive, even if it's mostly just used for ValueXY in the end, but otherwise I'd be wary of the complexity/maintenance cost relative to the value.

I'm not opposed though, especially since I probably won't have to worry about the maintenance :P, and the general refactoring looks nice.

@browniefed
Copy link
Member

I guess the one thing that was the main issue when dealing with arbitrary structures with animated values was mostly around the lack of logic to traverse a structure to find all the animated values.

I'm not sure creating complex animated structures was the issue. it was "I have an array for strokeDash and the logic can't see that it's an array and should look for animated values.". The second part of that I guess is what do you then ship over the wire with setNativeProps (or subsequent calls for other renderers). I assume the full prop combined w/ animated values and not animated values.

That being said I haven't thought through use cases of other renderers that may depend on complex structures, and to @sahrens point what are the maintenance costs going to be?

@sahrens
Copy link
Collaborator

sahrens commented Feb 29, 2016

For example, we use Animated values inside style transform objects which are similarly complex shapes and that works pretty smoothly.

@lelandrichardson
Copy link
Contributor Author

Yes, but for the style transforms we had to implement specific classes for this use case. We have AnimatedProps, AnimatedStyle, and AnimatedTransform classes.... all of which could theoretically go away in place of this if we wanted to.

I think for react native, the ability to have arbitrary shaped props utilize the animated API is very valuable. This was crucial to getting my maps component working with Animated, and I suspect it will be similarly useful for components in the ART library, gl-react, etc.

That said, it might be better to do this a different way... but I do think we should expose this freedom in some way.

@gre
Copy link
Contributor

gre commented Feb 29, 2016

Hi everyone.
@lelandrichardson pointed to me in #9 that I should weigh in here about my Animated needs, but I'm not sure if I shouldn't raise another issue, anyway i'm replying it here for now.


I have a more advanced use-case that I think is not covered here, and I'm not sure it can be covered in a "generic way". So I'm concerned about hidding the Animated/AnimatedWithChildren class in the library.

In gl-react, we have recently implemented support for "animated uniforms" that means Animated object over OpenGL Shaders. Animated is a huge benefit with its powerful API and it also increases the performance because removes the overhead of gl-react's internal "VDOM->Graph" transformations. I don't want to enter much into detail here, but basically gl-react resolves a big data object tree that contains the whole GL scene information. To make animated uniforms work, we have just allowed this data object to also contain Animated objects (at many levels).

In this old issue I first drafted my idea and we recently have it implemented in gl-react (but i've duplicated Animated/AnimatedChildren class and doing some duck typing for isAnimated so it obviously won't scale).

Let me take a simple gl-react example: here a 2-pass Blur:

on the left part is the Blur "usage" and then the impls of Blur and Blur1D and on the right part is the resulting data props (the "GL scene").

and the idea is that you can just pass-in a number OR an animated value in factor prop here:

<Blur factor={factor}>...</Blur>

which means that Animated Value will be at these different places

the object on the right is basically the data props that we want to "Animated-ify", and something need to "resolve" back to the initial data object (using __getValue()).

So I already have this implemented here and it works fine, but it's just not very "solid" as I've duplicated part of Animated library in gl-react.

I wish we can find on a way to solve this.

@lelandrichardson
Copy link
Contributor Author

@gre thanks for this context. In your example, does the base GL.Component handle the attaching of the animated props and deal with calling setNativeProps? Are you using createAnimatedComponent anywhere in order to do this at all?

I had some ideas to maybe create an opt-in API that a component could implement when it gets wrapped with createAnimatedComponent.

For instance, it could be something like:

var BlurAnimated = createAnimatedComponent(Blur, {
  getAnimatedProps: (props) => return { direction: props.direction }
});

Or something similar to that, where you could specify the props that were "white-listed" as potentially being animated values, and then methods similar to the helpers in Animated.Shape could handle passing those in through setNativeProps.

I'm not sure this is the exact right API, but maybe this angle is more helpful for your use case (and potentially mine as well).

@sahrens
Copy link
Collaborator

sahrens commented Feb 29, 2016

Ah right, I forgot we have a special class for transform.

It would be great if this could be more general infra that supported all the more specific instances.

On Feb 29, 2016, at 8:07 PM, Leland Richardson notifications@github.com wrote:

@gre thanks for this context. In your example, does the base GL.Component handle the attaching of the animated props and deal with calling setNativeProps? Are you using createAnimatedComponent anywhere in order to do this at all?

I had some ideas to maybe create an opt-in API that a component could implement when it gets wrapped with createAnimatedComponent.

For instance, it could be something like:

var BlurAnimated = createAnimatedComponent(Blur, {
getAnimatedProps: (props) => return { direction: props.direction }
});
Or something similar to that, where you could specify the props that were "white-listed" as potentially being animated values, and then methods similar to the helpers in Animated.Shape could handle passing those in through setNativeProps.

I'm not sure this is the exact right API, but maybe this angle is more helpful for your use case (and potentially mine as well).


Reply to this email directly or view it on GitHub.

@gre
Copy link
Contributor

gre commented Feb 29, 2016

Yes gl-react handles manually the setNativeProps calls, (inspired from how react-native does)
and I couldn't use createAnimatedComponent because it only handles shallow props (where I have a deep & recursive object (data props) where Animated.Value can exists).

_now here is a long explanation on the "technical detail", maybe just jump to the "maybe solution" if you don't want to read^^_

technical detail

Also AFAIK createAnimatedComponent assumes props are exactly mapped to the native props – so you can't internally transform the JSX to something else.

I can't use createAnimatedComponent on Blur because that Blur component is just an abstract React component that don't have a concrete DOM/Native rendering (like all gl-react components). <Blur/> is just an intermediary/temporary way to express a blur effect but it doesn't exists in the JSX VTree at the end: the GL Effects tree gets flattened into a data props (the gl scene) of the final Surface (either gl-react-dom or gl-react-native implementation) which ultimately is a GL component (a WebGL Canvas or a GLKView / GLSurfaceView) and the native implementation just need that GL scene as an input.

E.g.

<Surface>
  <A>
    <B />
  </A>
</Surface>

turns into something like

<GLCanvas data={{ shader: "a", uniform: { t: 1 }, children: [{ shader: "b", ... }] }} />

maybe solution

That is a technical choice on our side, but I feel Animated shouldn't prevent us to handle this. Actually does AnimatedShape support recursive inclusion of itself? Like an Animated.Shape({ foo: 1, bar: new AnimatedShape({...}), arrayToo: [ new AnimatedShape({ ... }) ] }) ?
I'm not sure if that could work but afterall I just need to express that my final GLCanvas's data props is a nested object with potentially some Animated nested inside, and I need the __getValue() to resolve the whole tree.

@lelandrichardson
Copy link
Contributor Author

@gre This is helpful context to the problems that different people might have.

Couple of things:

  1. Based on your example, it seems like to me that GLCanvas is the component that you would want to call createAnimatedComponent on, not Blur. Could be wrong about whether or not this will work directly with the way you've built these components.
  2. AnimatedShape does definitely support nested AnimatedShape properties. Based on it's implementation, it will just call .__getValue() on any prop that it finds that inherits from Animated (which includes AnimatedShape).

@gre
Copy link
Contributor

gre commented Mar 3, 2016

@lelandrichardson sounds good! I should probably give a try then :)

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.

4 participants