-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add culling #8089
Add culling #8089
Conversation
I like this addition. It's simple and doesn't require other dependencies. But it needs tests, particularly it would be useful for covering the use-case you described with filters. |
While I was working on the tests I noticed that my initial implementation wasn't doing what I promised (that it works correctly with filters always). I never noticed because in my own use case I only ever added culling to leaf nodes. On leaf nodes it does work with filters, but not if the cullable container has a child with a padded filter. We cannot skip the children even if the bounds do not intersect the source frame because we do not know if there's a child with a padded filter. That I fixed in the last commit. Other changes: I added the |
I added a few tests. I probably didn't cover every case. Filtered containers with |
@GoodBoyDigital and I had a chat about this. Our biggest concern was understanding how meaningful an optimization this is. Can you show a performance test with a deeply-nested and flat scenes with many objects (1-100K)? Where does |
Playground: https://www.pixiplayground.com/#/edit/6YshCmTQnz1q35pGum1c0 (Use the dev tools FPS meter) Deeply-nested vs flat should be more or less the same in this implementation (we cannot skip the children because of potential padded filters). This implementation doesn't have a problem with padded filters, and the user doesn't have to do anything more than set cullable to true. With the plugins the culling system needs to be run (manually) before rendering with the correct bounds of the current render target dimensions. The plugins probably have some speed advantages because they ignore the filter padding problem, but I haven't benchmarked this implementation vs the plugins yet. Culling is always nice to have for expensive objects that are not batched. But even for batched sprites there's a point where culling makes a noticeable FPS difference. |
The advantage of the plugins is that they can cull static objects efficiently, but it's the user's responsibility to update the culling system when it's necessary. But they don't have an advantage in culling dynamic objects that need to be tested every frame; here the performance of I updated the playground and added options to compare with pixi-cull and pixi-essentials/cull. pixi-cull doesn't seem to calculate the bounds correctly: the objects are hidden before they leave the frame. If you set the culling method to pixi-essentials/cull and enable filters and increase the padding, you can observe the filter padding problem. The cullable method is not affected. |
Here are some rough numbers based on the demo (2x, no filters). This was run on a 2019 macBook pro in the latest Chrome. All values here are approximate FPS from devtools. I think 2x is a good stress test because of the high fill-rate.
Few observations:
|
This looks good glossing over the code. I'll battle test it this weekend locally. |
thats not a bad improvement! |
@dev7355608, another future optimization that we could do for cullable, if you're open, is something similar to |
@dev7355608 could you please merge |
Great work @dev7355608, I think people will find this convenient and useful. |
Description of change
I propose to add culling to
Container.render(Advanced)
. I know there are@pixi-essentials/cull
andpixi-cull
, but they fail to take into account filter padding as far as I know: an object with a filter that has a large padding is hidden too early (filter padding is not included in the bounds of the object). This implementation of culling does not have this problem. It is also quite simple. Culling is only done ifcontainer.cullable
is true.Pre-Merge Checklist
npm run lint
)npm run test
)