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.

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

Combine State and NextState #7940

Closed
lewiszlw opened this issue Mar 7, 2023 · 4 comments
Closed

Combine State and NextState #7940

lewiszlw opened this issue Mar 7, 2023 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR

Comments

@lewiszlw
Copy link
Member

lewiszlw commented Mar 7, 2023

What problem does this solve or what need does it fill?

System may need receive two params to process one state (a bit redundant and counterintuitive). Is it possible that combining State and NextState into one?

@lewiszlw lewiszlw added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 7, 2023
@hymm
Copy link
Contributor

hymm commented Mar 7, 2023

here's some discussion from the Stageless RFC about this

maniwani on Aug 23, 2022
We initially had a consolidated resource, but anecdotally some users said they liked how iyes_loopless has them split.

The split just has the advantage that one system can queue a transition without blocking another system that just wants to read the current state.

Member
@maniwani maniwani on Aug 24, 2022 •
Oh, the more important property with split resources is that CurrentState isn't marked as changed when you queue a transition. (#2343)

Member
Author
@alice-i-cecile alice-i-cecile on Aug 24, 2022
The change detection argument is very compelling to me.

Member
@cart cart on Aug 25, 2022
Hmmm ok yeah theres serious value in being able to detect state changes. Can we change it to State instead of CurrentState for some additional terseness? I think "current" is implied.

Alternatively, we could just use interior mutability for State::set_next(). Given that state transitions will happen approximately once per frame, the overhead (even if we use a mutex) won't matter at all.

@alice-i-cecile alice-i-cecile on Aug 25, 2022
I like the rename. I don't love the interior mutability though because it breaks change detection for NextState.

Less essential, but likely still useful.

@lewiszlw
Copy link
Member Author

lewiszlw commented Mar 7, 2023

Hmmm, make sense. What about Query-like feature to combine resources?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 7, 2023
@alice-i-cecile
Copy link
Member

The SystemParam derive macro works nicely for that :)

I'm leaning towards "this won't actually be an improvement" because of the change detection argument, so are you okay to close this out?

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 7, 2023
@lewiszlw
Copy link
Member Author

lewiszlw commented Mar 7, 2023

OK, I'll look at how SystemParam works.

@lewiszlw lewiszlw closed this as completed Mar 7, 2023
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

3 participants