pixijs / pixi-react

Write PIXI apps using React declarative style
https://pixijs.io/pixi-react/
MIT License
2.34k stars 177 forks source link

Do not destroy children which are not created by react #303

Closed adsko closed 3 years ago

adsko commented 3 years ago

Description

Currently, a code to remove a component from a tree looks like this:

  if ((_child$children = child.children) !== null && _child$children !== void 0 && _child$children.length) {

    _toConsumableArray(child.children).forEach(function (c) {
      _removeChild(child, c);
    });
  }

  parent.removeChild(child);
  child.destroy();

It makes an issue with pixi components, which manages children on its own. See issue: https://github.com/pixijs/spine/issues/389

Pixi spine makes children on its own and destroy them in destroy method. Because of it, there is no way to manage component like this in react-pixi, because react-pixi will call destroy for each child and call destroy on the spine component.

I think, there should be a way to manage situation like this.

Steps to reproduce

  1. Make component like this:

    export const SpineComponent = PixiComponent<{ data: ISkeletonData }, OrigSpine>('Spine', {
    create: (props) => {
        const s = new Spine(props.data);
    
        s.state.setAnimation(0, 'idle_1', true);
    
        return s;
    },
    didMount: (instance, parent) => {
    },
    willUnmount: (instance, parent) => {
    },
    applyProps: (instance, oldProps, newProps) => {
        return true;
    },
    });
  2. Use the component
  3. Destroy component from the tree

Additional info

"pixi.js": "6.0.4",
"pixi-spine": "^3.0.3",
"react": "^17.0.2",
"@inlet/react-pixi": "^6.5.2",
"react-dom": "^17.0.2",
inlet commented 3 years ago

Fixed in new release, see Docs - Custom Component. All you need to do is set preventAutoDestroy: true and you're good to go

adsko commented 3 years ago

@inlet I think, children destroy also should be affected by this flag.

We don't know if everything is added to children (some container, sprites etc may be in memory).

inlet commented 3 years ago

This is also handled by the reconciler, see

https://github.com/inlet/react-pixi/blob/4b81c3f28960a7dfd6add578460887c0e6e29f69/src/reconciler/hostconfig.js#L33-L38

adsko commented 3 years ago

@inlet But you assume that every pixi component to destroy is in children array. Some of them may be in memory, see: https://github.com/pixijs/spine/blob/b50a4bfc01be727f86b74ddb2963b0363f5d8a3f/packages/base/src/SpineBase.ts#L745

Some containers and sprites are stored in class fields. Because of it, there may be memory leak if we enable "preventAutoDestroy".

It would be nice to have second flag like: "preventChildrenDestroy": boolean; And with these two flags, we can manage almost all cases.

inlet commented 3 years ago

what about we provide a config object like this?

{
  config: {
   destroy: false,
   destroyChildren: false
  },
  create: () => {},
  applyProps: () => {},
  willUnmount: () => {}
}
adsko commented 3 years ago

@inlet It makes sense to group it in object.

inlet commented 3 years ago

Adding a patch now...

inlet commented 3 years ago

done! see https://github.com/inlet/react-pixi/releases/tag/v6.6.1

inlet commented 3 years ago

Hopefully this fix helps you