Skip to content

Conversation

TheRawMeatball
Copy link
Member

Remove the Stage trait to simplify internals, and make further work on #1375 and adjacent tasks easier. Startup schedules, which were the only place where the api was used have been moved to a separate field in App.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 6, 2021
@TheRawMeatball TheRawMeatball added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Sep 6, 2021
@alice-i-cecile alice-i-cecile self-requested a review September 6, 2021 14:59
@cart
Copy link
Member

cart commented Sep 6, 2021

Given that this "reels in" useful functionality in preparation for a new design, I think I'd want an actual plan for what that design is prior to merging this. Otherwise how do we know whether or not these changes are enabling the right things?
I'm also not convinced that these changes make future changes much easier. Anything that changes how stages work will need to touch the majority of these lines again.

@TheRawMeatball
Copy link
Member Author

The next step moving forward would most likely be to relocate run criteria storage and eventually evaluation into Schedule, resolving issues around not being able to use a state from multiple stages, and having hard sync points within blocks controlled by run criteria. @Ratysz might have further insight here.

I'm pretty confident having a locked-in stage topology will make moving run criteria to a central location easier, which is why I'm doing this PR first. We could postpone until more work is done, but that might lead to a rather bloated PR that's tricky to review.

@Ratysz
Copy link
Contributor

Ratysz commented Sep 6, 2021

I think existence of trait Stage is largely incosequential for all changes up to the one that deprecates stages as a concept. We'll have to contend with them one way or another up until we remove them entirely, trait or not.

@TheRawMeatball
Copy link
Member Author

Closed in favor of waiting for bigger changes with #2801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants