pixijs / pixi-react

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

Any way to run destroy() of PIXI.Sprite? #346

Closed MorookaKotaro closed 2 years ago

MorookaKotaro commented 2 years ago

Description

I have this following code, but it causes memory leaking only in Safari, since the Sprite is completely removed. (seems GC not working)

return (
  <Sprite
    texture={loader.resources[images[index]].texture} // index is set constantly based on user action
  />
)
image

I simulated the same situation with raw PIXI.js. I guess, since the destroy method is running, the memory is not leaking. https://codesandbox.io/s/staging-architecture-tl7v0x?file=/src/index.ts

I'd like to know how to use destroy method with react-pixi, or other way to avoid memory leaking.

Additional info

inlet commented 2 years ago

Hi @MorookaKotaro,

Thanks for filing this issue.

I'm not sure if I understand your question correctly. Under the hood when a <Sprite> component unmounts it will automatically be destroyed, see:

https://github.com/inlet/react-pixi/blob/84559fec8ddf43674a2987a98fe2dacc6949d00b/src/reconciler/hostconfig.js#L43

So I'm not quite sure where this memory leak is coming from, can you create a codesandbox demo with the memory leak with react-pixi? Thanks

inlet commented 2 years ago

On the other hand, in your code above the Sprite never gets destroyed because it will never unmount. Which is good.. you are only changing the texture.

It might be possible that there's something else happening in your code that's causing the memory leak?

MorookaKotaro commented 2 years ago

Hello @inlet

Thank you for replying. Okay, I'll create react-pixi demo in a moment.

Sprite never gets destroyed because it will never unmount.

Yes, that's exactly what I was thinking. Since only the texture changes, Sprite will never unmount. Which means new Sprite object is created every time as the texture changes, though the older ones are not removed. That's why I was looking for the way to run destroy method manually.

Anyway, I'm going to create the demo!

inlet commented 2 years ago

Which means new Sprite object is created every time as the texture changes, though the older ones are not removed. That's why I was looking for the way to run destroy method manually.

Only 1 Sprite object will be created only the texture member will be updated, same as sprite.texture = newTexture

MorookaKotaro commented 2 years ago

@inlet

Hmm.. It seems working well with minimum demo... https://codesandbox.io/s/frosty-darkness-wvs8ds?file=/src/index.tsx

It could be my source code's fault. Sorry for opening a mistaken issue :_(

I'm going to investigate more, then I will contact you if anything founded.

Thank you.

inlet commented 2 years ago

Sure, keep me posted so I can close this issue. Thanks!

MorookaKotaro commented 2 years ago

Hello @inlet I may found the issue. As the total image size is larger, the memory seems never released in Safari. image

On the other hand, in Chrome, JS Heap size works well. image

I would appreciate it if you could check my demo.

demo: https://codesandbox.io/s/jolly-water-equs3i

inlet commented 2 years ago

I don’t see any issues with your demo, it must have to do something with how Safari handles GPU GC.

It might be worth filing an issue in the PIXI repo? Have you tried it with vanilla PIXI?

MorookaKotaro commented 2 years ago

I don’t see any issues with your demo

It might be because of machine's speck. I'll make it load more image, then you could see the issue.

Have you tried it with vanilla PIXI?

Yes. The first one is with vanilla. When the destroy method is not called, the memory graph looks similar as the one with react-pixi https://codesandbox.io/s/staging-architecture-tl7v0x?file=/src/index.ts

inlet commented 2 years ago

Right so we can confirm the same logic happens with vanilla PIXI.

I’ll close this issue as it’s not related to react-pixi but with PIXI in general

MorookaKotaro commented 2 years ago

Wait, It can be avoided calling destroy method as a texture changes with vanilla. So I guessed destroy method is not called properly in react-pixi.

inlet commented 2 years ago

Have you tested it out with Sprite that’s getting removed? As you can see in the reconciler tests all unmounted components are getting destroyed properly.

https://github.com/inlet/react-pixi/blob/master/test/reconciler.test.js#L309

inlet commented 2 years ago

In your example you’re updating the texture prop, please try setting a key prop that unmounts the old and mounts a new one

MorookaKotaro commented 2 years ago

@inlet Yesss, it works with key prop!!!!! Thank you so much😭

MorookaKotaro commented 2 years ago

@inlet FYI: Although it actually works with following code to some extent, it's necessary to set a key prop on Stage component for much heavier images.. (By the way, it didn't work to set a key prop on Sprite component)

<Contaienr key={state}>
  <Sprite
    ...
    texture={state}
  />
</Container>
inlet commented 2 years ago

Thanks for the reference 😁