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

HostConfig.insertBefore not emitting __REACT_PIXI_REQUEST_RENDER__ thus no rerender #408

Closed
jsimomaa opened this issue Feb 9, 2023 · 8 comments · Fixed by #470
Closed
Assignees

Comments

@jsimomaa
Copy link
Contributor

jsimomaa commented Feb 9, 2023

Current Behavior

When having a conditional child for a <Container> within a <Stage> the child does not reappear once removed.

Expected Behavior

The blue line should become visible again after clicking the button again.

Steps to Reproduce

HostConfig.insertBefore not emitting __REACT_PIXI_REQUEST_RENDER__ when a new child is inserted in to a <Container> within a <Stage raf={false}>:

  ...
  const toggledRect = toggled ? (
    <Rect x1={300} y1={300} x2={300} y2={150} />
  ) : null;

  return (
    <Stage raf={false} renderOnComponentChange={true}>
      <Container>
        {toggledRect}
  ...

See reproducible example at. The behavior can be seen when clicking the button :
https://codesandbox.io/s/distracted-dust-4t4erp?file=/src/App.tsx

Environment

From the above sandbox:

  "dependencies": {
    "@pixi/core": "7.1.2",
    "@pixi/react": "7.0.1",
    "pixi.js": "7.1.2",
    "react": "18.0.0",
    "react-dom": "18.0.0",
    "react-scripts": "4.0.3"
  },

Possible Solution

All the other HostConfig functions that deal with mutation of children do emit __REACT_PIXI_REQUEST_RENDER__ and so should insertBefore as well in order to rerender the changed stage:

The relevant part in the code:

function insertBefore(parent, child, beforeChild)
{
invariant(child !== beforeChild, 'pixi-react: PixiFiber cannot insert node before itself');
const childExists = parent.children.indexOf(child) !== -1;
const index = parent.getChildIndex(beforeChild);
childExists ? parent.setChildIndex(child, index) : parent.addChildAt(child, index);
}

@baseten baseten self-assigned this Feb 10, 2023
@jsimomaa
Copy link
Contributor Author

Any comments if this actually is a bug and can be fixed with the solution below:

 function insertBefore(parent, child, beforeChild) 
 { 
     invariant(child !== beforeChild, 'pixi-react: PixiFiber cannot insert node before itself'); 
  
     const childExists = parent.children.indexOf(child) !== -1; 
     const index = parent.getChildIndex(beforeChild); 
  
     childExists ? parent.setChildIndex(child, index) : parent.addChildAt(child, index); 
     parent.__reactpixi?.root?.emit(`__REACT_PIXI_REQUEST_RENDER__`, { detail: 'insertBefore' });
}

Do you accept a PR for this?

@rnike
Copy link

rnike commented Sep 5, 2023

We also encounter this issue, the solution @jsimomaa given does fix the bug.

@avpeery
Copy link

avpeery commented Jan 10, 2024

@jsimomaa I believe I ran into this same bug #469. I think pixi-react team should put in a fix for this rather than us implementing a workaround in the hostConfig file? Have you opened a PR yet? bumping up @baseten @bigtimebuddy thanks all!

@jsimomaa
Copy link
Contributor Author

jsimomaa commented Jan 10, 2024

It seems like the development of this project halted after it become an official PixiJS package. It would be nice to get some kind of a comment from the PixiJS team about the status and the future of this project @baseten @bigtimebuddy @Zyie

The main pixijs project seems to be active but this React package is falling behind

Thanks!

@Zyie
Copy link
Member

Zyie commented Jan 10, 2024

Hey @jsimomaa

We're a small team juggling priorities, and the v8 release for Pixi has been keeping us busy for a year now.
We're planning to get the React package up to speed too with the v8 launch.

In the meantime this is an open source project, so If you're keen to chip in, feel free to submit PRs – we'd love the help!

@jsimomaa
Copy link
Contributor Author

Hi @Zyie

Thank you for a quick response! Nice to hear that this React package is still maintained and will be brought up to speed with the v8!

I'll submit a PR for this issue ASAP like proposed in this comment: #408 (comment)

@jsimomaa
Copy link
Contributor Author

Hi @Zyie please see this PR: #470

Thanks

@OwenTrokeBillard
Copy link

A workaround is to add onglobalpointermove={() => null} to your container. The callback will not be invoked as long as the container does not have interactive={true}.

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 a pull request may close this issue.

6 participants