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

renderOnComponentChange being ignored? #78

Closed
bichotll opened this issue Jan 28, 2019 · 17 comments
Closed

renderOnComponentChange being ignored? #78

bichotll opened this issue Jan 28, 2019 · 17 comments
Labels
good first issue Good for newcomers

Comments

@bichotll
Copy link
Contributor

Description

renderOnComponentChange ignored?

I have the following event firing eventho I have set renderOnComponentChange to true

 this.appRef.current.app.renderer.on('postrender', () => {
  console.log('./postrender', +(new Date()))
    })
  }

I expect the app not to render all the time.

Demo (check console)

https://codepen.io/anon/pen/yZJXdG#anon-login

@bichotll
Copy link
Contributor Author

btw, I'm happy to help there if that is actually a bug.

I will need some tips to find out what it can be causing this issue.

@inlet
Copy link
Collaborator

inlet commented Jan 29, 2019

This only works in combination with raf set to false.

@bichotll
Copy link
Contributor Author

bichotll commented Jan 29, 2019 via email

@inlet
Copy link
Collaborator

inlet commented Jan 29, 2019

The renderOnComponentChange prop isn't used very often.

btw, that d just work whenever there is a change in the
component, right? Not in any child or grand child..

True, that's why this prop is quite useless. Maybe we need to remove this prop entirely.

The following may be a bit offtopic. Is there any way to detect a change in
a grand child? Actually, is not changes to Pixi?

Stage updates the reconciler with children then the reconciler takes over. A React parent component is not aware of component updates in children, this is of course intended.

However the reconciler itself is aware of add/remove/change props.

@bichotll
Copy link
Contributor Author

The renderOnComponentChange prop isn't used very often.

Right. It would be nice if it detected any change.

Stage updates the reconciler with children then the reconciler takes over. A React parent component is not aware of component updates in children, this is of course intended.

I see now. I got confused..I thought that Stage was communicating to the reconciler from what I saw on the code. Therefore, the events would go through it?

However the reconciler itself is aware of add/remove/change props.

Right, what would be the best/cleanest way to communicate with the reconciler to know when there is a change?

My point is: it would be good (and at some point a battery saver) if the reconciler communicated Pixi to render whenever it is necessary.
could have the property raf and renderOnChange. If renderOnChange is true, then Stage would listen the reconciler and would ask the pixi Application instance to render.
Does that make sense?

Besides that, I actually needed to know when Pixi was running the render process.

@inlet
Copy link
Collaborator

inlet commented Jan 31, 2019

Totally agree with you, it would be a good practise to update the renderer whenever a Pixi prop is changed. The reconciler is isolated and is not directly aware of any renderer (which is a good thing), we need to find out if there's an easy way to retrieve the renderer in order to update it.

Feel free to create a PR with the implementation :) I'd love to do it, but I won't have time on short term.

Else I would be glad to work on this together when we both have time!

Thanks

@bichotll
Copy link
Contributor Author

I have other priorities atm, but I'll keep you posted if any update on the subject :) 👍

Cheers

@inlet inlet added the good first issue Good for newcomers label Feb 13, 2019
@bichotll
Copy link
Contributor Author

I'm a bit frustrated 😅. I could not find any way to listen to react-reconciler events. Plus, the package is barely documented.

I'll try to approach this issue from another perspective. For now, I think that the best approach is using the context, accessing the Pixi app instance and calling the render method.

There may be a possibility to implement a hoc.
Just an idea re hoc, we could try to *Proxy the child's componentDidUpdate method and then call the render method of the Pixi app instance.
*https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

Let me know your thoughts 👍

btw, I see that react-pixi-fiber has the same issue:
michalochman/react-pixi-fiber#48 (comment)

@inlet
Copy link
Collaborator

inlet commented Feb 27, 2019

Haha yes I can imagine, the reconciler is actually an abstracting layer on top of your components. There is no clean and elegant way to listen for events.

I'll try to approach this issue from another perspective. For now, I think that the best approach is using the context, accessing the Pixi app instance and calling the render method.

Yes that would work, but then you have to manually render the stage.

There may be a possibility to implement a hoc.
Just an idea re hoc, we could try to *Proxy the child's componentDidUpdate method and then call the render method of the Pixi app instance.

ComponentDidUpdate doesn't tell you anything about which props on which components are changed. This problem is not easy to solve, but I guess you already figured that out ;)

The Stage creates a PIXI.Application containing the renderer. We need to pass down the renderer in the reconciliation to its children on mount. When applying the props it needs to re-render the stage.

I don't have time to look into this within the upcoming weeks, but will look into it as soon as possible.

@darklightblue
Copy link

darklightblue commented Jul 19, 2019

Hello, any updates about this? I'm having an issue where I want to change the source spritesheet of my custom animated sprite component when someone clicks a button or performs an action - but I can't do it coz it can't detect if something changed. :(

I can't turn off raf too coz it will stop the sprite from animating.

This refers to your second discussion btw

@inlet
Copy link
Collaborator

inlet commented Jul 19, 2019

Feel free to create a PR 😊 this is still not implemented.

Although I believe that what you want is not related to this issue. Just change the source prop and you’re good to go. Create a Custom Component and you can manually catch prop changes and basically do anything you like.

@fabienjuif
Copy link
Contributor

@inlet can you telle me if I understand the issue topic

Right now:

  1. the reconcilier role is just to map react props change to pixi real engine
  2. application is rerendered at every frame (+- 60fps), even if there is no modification in children (in DisplayObjects)

What we want is that pixi rerender the tree only when a "pixi react component" prop is modified

Did I understand?

@inlet
Copy link
Collaborator

inlet commented Jul 19, 2019

Well the reconciler does a lot more, but basically the Stage creates a PIXI.Application which starts a ticker (raf) and rerenders the stage every frame.

However the reconciler is isolated and is not aware of any renderer, it just creates and modify PIXI primitive components. So we have to find an elegant way to access the renderer within the reconciler component tree.

Also, the internal ticker is optimized so you don’t want to disable it by default.

@darklightblue
Copy link

After a couple of hours figuring out what to do.

I realized that I can use applyProps and put my PIXI js code - you can actually change/access the "child" via instance (first argument in applyProps).

Thanks by the way!

@inlet
Copy link
Collaborator

inlet commented Jul 19, 2019

@darklightblue you’re welcome! glad you figured it out 😉

@inlet
Copy link
Collaborator

inlet commented May 25, 2020

Finally got time to implement this feature 🙌

Only need to add some tests and update the docs accordingly.

@inlet inlet closed this as completed May 26, 2020
@bichotll
Copy link
Contributor Author

Amazing! Thank you! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants