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.

Sign up for GitHub

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

Jump to bottom

Fusion.Children should parent things in batching order #293

Open
boatbomber opened this issue Dec 14, 2023 · 7 comments · May be fixed by #298
Open

Fusion.Children should parent things in batching order #293

boatbomber opened this issue Dec 14, 2023 · 7 comments · May be fixed by #298
Labels
enhancement New feature or request ready - closing soon All done, waiting on merge

Comments

@boatbomber
Copy link
Contributor

boatbomber commented Dec 14, 2023 •
edited
Loading

Roblox's renderer batches drawcalls using a naive greedy check that depends on iteration order, which is defined by the order in which the children are parented. It's very frustrating, and I've documented it here:
https://devforum.roblox.com/t/renderer-doesnt-batch-drawcalls-if-ui-elements-arent-parented-in-order/2740804

Hopefully Roblox will improve this behavior, but I fear it may be quite some time until then. For now, an easy uplift on our end is to modify the behavior of Fusion.Children to parent the given children in batching-friendly order. The change is straightforward- instead of child.Parent = applyTo, we binary insert child to a sorted array and just iterate over it and parent them at the end.

I won't open a PR with this yet, but I have implemented it and have been testing it in a local branch of Lua Learning. I wanted to open an issue here so we can discuss if this is something we do want to support.
@boatbomber boatbomber added enhancement New feature or request not ready - evaluating Currently gauging feedback labels Dec 14, 2023
@boatbomber
Copy link
Contributor Author

boatbomber commented Dec 14, 2023 •
edited
Loading

Note that this change doesn't "solve" batching drawcalls, just some cases. If your images are nested inside wrapper frames, this change has no impact, for example. Your mileage may vary, but if your tree is pretty flat this can have some real benefit.

@dphfox
Copy link
Owner

dphfox commented Jan 2, 2024 •
edited
Loading

Since discovering that ZIndex can control batching without the library having to add hardcoded behaviour for this, is it necessary for us to account for this detail ourselves?

@boatbomber
Copy link
Contributor Author

boatbomber commented Jan 2, 2024 •
edited
Loading

I would argue that yes, it does still help. Managing ZIndexes to layer things for batching is a headache to do in every component (you'd need to create your Children array in parenting order, which is annoying and unwieldy), and there are cases where you can't do that anyway.

@boatbomber
Copy link
Contributor Author

Since making this issue, I've pushed this implementation to production and have had no problems with it. My drawcalls decreased by 20%, although that does include some additional work I did to restructure some trees to benefit from this.

@boatbomber boatbomber linked a pull request Jan 2, 2024 that will close this issue
Open
@boatbomber
Copy link
Contributor Author

I opened the PR to show what that implementation looks like, but if we decide not to do this then I don't mind closing it. I do believe that this simple change provides value though.

@dphfox dphfox added not ready - blocked Waiting on other work to be done and removed not ready - evaluating Currently gauging feedback labels Jan 2, 2024
@dphfox
Copy link
Owner

dphfox commented Jan 2, 2024 •
edited
Loading

I mentioned this in the PR, but I'm going to defer on this for a while until we know whether Roblox themselves plan to address this soon.

@dphfox dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - blocked Waiting on other work to be done labels Apr 15, 2024
@dphfox
Copy link
Owner

dphfox commented Apr 15, 2024

Unblocking this; see #298.

@dphfox dphfox added ready - closing soon All done, waiting on merge and removed ready to work on Enhancements/changes ready to be made labels Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request ready - closing soon All done, waiting on merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Parent children in batchable order when possible boatbomber/Fusion
2 participants
@dphfox @boatbomber