System builder syntax rework #31
Conversation
Yep! This change is only going to get more painful the longer we wait. |
|
IMO there's an important bit of Future work that should be included: this would provide an excellent unified foundation for "label descriptors", a much-discussed future feature that would allow users to specify schedule and run criteria information on labels, which would then be dispatched to each system with that label. |
By included do you mean the changes required for label descriptors be made on top of this pr before merging it? If so, might it be easier to handle 2 smaller prs rather than one massive one? |
|
"Future work" refers to a subsection of the RFC where you note down stuff that could happen after this RFC is accepted and implemented. I don't think label properties are relevant here, though. It's mostly an orthogonal feature, in both API and potential implementation. |
|
Alright, I'm happy to accept @Ratysz's judgement on that. |
|
I have a couple of ideas about the
I'm quite ignorant to that part of the codebase so I don't know if the proposed APIs are possible or the implementation effort broaden too much the scope of the RFC/PR. |
I'd be quite happy with this, although I'd prefer |
That was a while ago, I've fixed that. This breaks it again; haven't had the time to try and fix it yet. |
|
I didn't even try to get rid of |
| Mostly described in [#1978](https://github.com/bevyengine/bevy/issues/1978). | ||
|
|
||
| - Allows us to unify the existing `.label()`, `.after()`, etc. methods and the stage positioning of systems. | ||
| - App would only have 3 methods to add systems: `add_system`, `add_exclusive` and `add_system_set`. |
There was a problem hiding this comment.
I'm not convinced that giving exclusive systems their own api / making them more separate is the direction we should be going in (at least, not yet). Exclusive systems are just systems with arbitrary side effects (new archetypes, new components, etc). Our plans for "stageless schedules" need to have some way to accommodate "normal system" side effects (ex: applying Commands). I personally think solving that problem well will involve blurring the lines between exclusive and normal systems, potentially to the point that distinguishing between the two isn't worthwhile. Given that we don't yet know what this will look like in practice, I'd rather stick with what we currently have (which also happens to align better with the future that I think is coming). @Ratysz what are your thoughts on this?
There was a problem hiding this comment.
I think separation is absolutely the right way to go. In fact, this PR doesn't go as far as it probably should - a lot of the weird API and implementation somersaults had to happen precisely to accomodate exclusive systems.
We should stop trying to merge parallel and exclusive systems until we actually have the ability to do so (which is, indeed, a part of stageless), because right now we definitely can't blur the lines. Or so I keep saying.
There was a problem hiding this comment.
I forgot to mention: we tried here. It looks like a Rust limitation, it refuses to infer types across one of the boundaries.
There was a problem hiding this comment.
Haha I figured you'd take that stance. I don't have strong opinions here because exclusive systems are relatively uncommon and there are definitely good reasons to keep them separate in the current implementation. This change just creates more churn for existing exclusive system users and creates a situation where in the future we'll probably break them again in favor of something that will probably be closer to the current api. To me the (possibly temporary) marginal wins we get from this separation don't outweigh the cost of breaking everyone, but I won't push back too hard on this if everyone else thinks differently.
There was a problem hiding this comment.
I forgot to mention: we tried here. It looks like a Rust limitation, it refuses to infer types across one of the boundaries.
For what its worth, I made some changes to exclusive systems in #2777 that probably enable this.
There was a problem hiding this comment.
This change just creates more churn for existing exclusive system users and creates a situation where in the future we'll probably break them again in favor of something that will probably be closer to the current api. To me the (possibly temporary) marginal wins we get from this separation don't outweigh the cost of breaking everyone, but I won't push back too hard on this if everyone else thinks differently.
Don't get me wrong, I also subscribe to this line of thinking; it's too early to merge them, too late to not merge them - hence the dredged-up post. It's hard to not be a little bitter after time and time again seeing a shiny new API get completely blown up and polluted with faux specialisation and pseudo-HKT noise as soon as it needs to account for exclusive systems.
For what its worth, I made some changes to exclusive systems in #2777 that probably enable this.
Unlikely, the issue is the same that lead to enum SystemDescriptor: we need to have a concrete type somewhere between generic system and its consumer. Only this time the boundary is smeared across half a dozen traits with 3-4 implementors each.
There was a problem hiding this comment.
It's hard to not be a little bitter after time and time again seeing a shiny new API get completely blown up and polluted with faux specialisation and pseudo-HKT noise as soon as it needs to account for exclusive systems.
Yup I totally get that and concede the point that the current implementation would be much cleaner if we embraced a hard split. But from my perspective its hard not to be bitter that we keep investing in a "detour" in the interest of simplicity of the current executor. We wouldn't need to care about ugly faux specialization and pseudo-HKT noise if we just decided to treat exclusive systems as "normal systems" like we did in the old executor. Which I'm relatively certain is where we are headed in the future too. I pushed back pretty hard on the exclusive system split when the new executor landed and honestly its becoming clear to me that this split really is a "detour" that optimizes for a specific executor implementation that is lacking a feature that we all know we want. If cleanness and simplicity are the goals, imo our time is best spent on re-unifying exclusive systems and normal systems / formally acknowledging that Systems can have side effects (which again, is what the old impl did and what I pushed for in the new executor).
Im being a bit overly dramatic here. Not trying to be combative / turn this into an argument. I'm very happy with the improvements the new executor brought and we may very well have been better served by embracing fully separate exclusive systems in the short term. Just trying to illustrate that I also have some strong feelings about this situation / trying to qualify why I think investing further in the system/exclusive system split isn't what I want personally.
There was a problem hiding this comment.
For the most part, I agree; I don't think being conservative about splitting is unreasonable. I keep trying to advocate for it because I believe that the amount of headache it saves us in the short term is far, far greater than the amount of headache we will endure when mending this schism further down the line.
|
So how's this going. Has cart's prerequisite stuff been merged? |
|
Fixed by #45. |
Existing PR: #2736
Original issue: #1978