-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 labels to render systems #1511
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the label / system name discrepancy I raised is solved this looks good to merge.
6d39e85
to
429e67a
Compare
This seems to fix #1506 |
If it does, it's probably a matter of accidentally perturbing system orders not an actual fix. The only behavior change this PR should have is to force the visible entities system to run after the camera systems, which I think would have become un-ordered in the post-0.4 changes. But I don't see how that would be related to #1506, maybe it is though. |
I could be wrong but I believe #1506 is caused by the new material not being loaded before it needs to be rendered, which ordering |
Hmm. But this shouldn't explicitly effect the order of that system. All it does is add a label to it so other systems can order themselves relative to it in the future. |
Oh yeah... I guess I just got lucky with the random ordering after I applied this PR. 🤷 |
If we decide to go the same way with this PR as #1512 then I would really like it if we could keep the label on RenderSystem::VisibleEntities. I need some way to put Draws into the visible entity list for the camera which means I need to schedule around this system. Or maybe sit down with the rendering code and come up with a better solution, but adding stuff to visible entities was the easiest thing I could come up with. |
Unlike #1512, this PR also add some ordering which is needed, it's not a case of "just label everything by default". |
663e46c
to
2554629
Compare
I think "visibility calculation" is core enough to render logic that allowing people to insert before/after makes sense to me. I sort of want to wait and see how "multiple labels" pans out, as its now looking like theres a chance it will make it into 0.5. Can we make the labels in this pr private (and scoped only to things that need internal ordering), then prior to 0.5 we can make a pragmatic decision about label exports vs "multi-label" solutions? |
Ok, that makes sense. Can anyone give me some guidance on what needs internal ordering? |
I'll need to dig in to it. I was planning on taking a pass over this stuff anyway. I wrote a lot of these systems, so its probably more efficient for me to just go through them. |
Closing for now in favor of #1606. We can re-open or re-implement if we decide to label all internal systems. |
This create a RenderSystem enum, symmetric with other bevy crates, and labels all the render systems with it. This represents incremental progress towards #1466