Bug: Container Rendering Children in Wrong Order

[maximtwo]

Current Behavior

Howdy, thanks for taking a look at this report!

I've run into an issue where I have an array of layers in my application and the user is allowed to re-order the layers. When the layer order changes what is displayed in the scene is not the expected output. I think it may be a problem during reconciliation because when I generate new component keys each time the parent component re-renders everything is ordered correctly.

I've included a live example so that you can see what's currently happening.

Expected Behavior

I expect that when you have an array of objects to render inside of a Container, the order in which they are rendered to the screen is consistent with ordering of the array.

<Container>
  {layers.map((layer) => (
    <Graphics key={layer.id} draw={draw(layer)} tint={layer.tint} />
  ))}
</Container>

Steps to Reproduce

https://codesandbox.io/embed/modern-darkness-1tfqvt?fontsize=14&hidenavigation=1&theme=dark

• Note the initial rendering order: green square, red circle, blue rectangle
• Click anywhere on the canvas
• Note the rendering order is now changed but it is incorrect: red circle, blue rectangle, green square
• It should be red circle, green square, blue rectangle

Environment

• @pixi/react version: 7.0.2
• pixi.js version: 7.1.4
• React version: 18.2
• ReactDOM version: 18.2
• Browser & Version: Chrome v110
• OS & Version: Windows 10
• Running Example: https://codesandbox.io/embed/modern-darkness-1tfqvt?fontsize=14&hidenavigation=1&theme=dark

Possible Solution

As mentioned above, one possible solution was to generate new keys for each layer when the ordering changes but this is less than ideal because it can cause unnecessary re-renders of potentially large portions of the scene graph.

Additional Information

No response

[lunarraid]

I ran into this years back, and this is what I wound up with:

Current logic:

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);
}

My logic:

function insertBefore (parentInstance, child, beforeChild) {
  invariant(child !== beforeChild, 'pixi-react: PixiFiber cannot insert node before itself');

  const childExists = parentInstance.hasChild(child);

  if (childExists) {
    parentInstance.removeChild(child);
  }

  const index = parentInstance.getChildIndex(beforeChild);

  parentInstance.addChildAt(child, index);
}

[michalochman]

@lunarraid your solution looks correct. It was also fixed in similar way in my original lib 3 years ago by one of the contributors here: https://github.com/michalochman/react-pixi-fiber/pull/98

@baseten @Zyie I think we can safely migrate it to pixi-react. It is probably a good idea to go through all fixes that were made along the way since "forking" and migrate those as well.

[maximtwo]

Hey guys, thanks for digging into this, I was going to open a PR with @lunarraid 's suggested fix and an additional unit test that catches this specific issue. Reading the comment above though, it sounds like there may be other issues that you want to address too.

Should I hold off on opening that PR?

[lunarraid]

@maximtwo I believe he is referring to other github issues that were opened and patched in the original library that was forked, and is suggesting that those patches should be looked through to see if any haven't been applied to the fork. A PR for this issue with fix and tests would be great.

[maximtwo]

Will be incoming shortly, thanks for the guidance

[baseten]

@maximtwo thanks for the PR - this fix is now released in pixi-react v7.0.3