Spring animations not updating (react-babylon-spring)

[flostellbrink]

I've been trying to get react-babylon-spring working. Not sure if any of this is related to #115

From what I can tell the nodes that are passed through applyAnimatedValues don't actually have the propsHandler that applyInitialPropsToCreatedInstance expects. It looks like instead of createdInstance those are the host nodes directly from babylon js.

Anyway I am having a hard time wrapping my head around how this is supposed to work.

So I set up the storybook locally and its broken in the exact same way where animations will only play while some hook is updating the components itself.

https://user-images.githubusercontent.com/2600682/134780574-376d6800-935f-4755-a108-be44be507c89.mov

The official storybook still works, but then again it hasn't been updated in a year, so I am guessing something actually broke.

[flostellbrink]

Deployed the latest storybook to my fork, so its easier to compare:

• Latest broken storybook
• Old working storybook

[brianzinn]

Thanks Florian. It’s been broken for some time. I’ll have a look tomorrow to try to fix - I need a fix that is backwards compatible as this library has undergone a major update since the last NPM of rbs was published.

[flostellbrink]

Thanks for taking time to look into this Brian. Let me know if there is anything I can do to help!

[brianzinn]

ok, I did find the issue and have a fix. you were spot on with your diagnosis of the root cause. if you change one method then the playground example will work:

export const applyInitialPropsToInstance = (hostInstance: any, props: any): void => {
  const propsHandlers: HasPropsHandlers<FiberAbstractMeshProps> = new FiberMesh();

  const createdInstance: CreatedInstance<any> = {
    hostInstance,
    propsHandlers
  } as CreatedInstance<any>;
  applyPropsToRef(createdInstance, props);
}

That is a terrible solution not only because it will only work for Mesh objects, but it's going to create even more GC pressure on those animations. So, I'm going to attach a property to the babylon object with a ref to the propsHandlers and will fix those methods accordingly - actually move them to a new file as they are for external use. I've always wanted to avoid adding anything to the babylonjs objects as I didn't want to "pollute" it with internal workings of this project, but it seems the best solution right now...

For reference also this was an unintended breaking change created when the API was "improved" on the public reference from the renderer. So, when you went like this:

 const sphereRef = useRef();
 if (sphereRef.current) {
   // v3 sphereRef.current is a mesh - this seems more intuitive
   // v2 sphereRef.current is a `CreatedInstance<T>` and sphereRef.current.hostInstance is a mesh
 }
 ...
    <sphere ref={sphereRef} />
...

The change that did that was in the renderer HostConfig<> method getPublicInstance.

Not sure if you wanted all the gory details, but there they are. I'll have a fix out today and also merge the storybook action - thanks again for that!

[flostellbrink]

This makes so much sense now. Thanks for explaining it!

It's basically this line, right? Really like that behaviour 👍

Instead of having a ref to the propsHandler, could that be a ref to the instance itself on the babylon objects?

[brianzinn]

I was planning something dynamic like Object.defineProperty(...): https://github.com/brianzinn/react-babylonjs/blob/master/src/ReactBabylonJSHostConfig.ts#L514

It will be easy to change later as it will be an internal unsupported property - if there is a proposal for a better idea which doesn't affect the public API or cause too much GC pressure on the animations I would welcome that.

[flostellbrink]

Oh right that's pretty neat.

I mean react-dom does something similar where they put a __reactFiber on the actual dom objects.

So I guess having something like that is probably necessary if the public instances are babylon objects.

[brianzinn]

I don't know if you wanted to have a look at that branch to confirm it works as expected for you first, but otherwise I can just merge your PR and then that branch 😃

[brianzinn]

I went ahead and merged it and also your PR and all the storybook pages are live! are you OK with a mention on the main readme?

[flostellbrink]

Really neat to see it working again. Great job 🚀 Thank you so much!

Would definitely appreciate a mention!