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

Remove States::variants and remove enum-only restriction its derive #9945

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Sep 27, 2023

Objective

The States::variants method was once used to construct OnExit and OnEnter schedules for every possible value of a given States type. Since the switch to lazily initialized schedules, we no longer need to track every possible value.

This also opens the door to States types that aren't enums.

Solution

  • Remove the unused States::variants method and its associated type.
  • Remove the enum-only restriction on derived States types.

Changelog

  • Removed States::variants and its associated type.
  • Derived States can now be datatypes other than enums.

Migration Guide

  • States::variants no longer exists. If you relied on this function, consider using a library that provides enum iterators.

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Sep 27, 2023

Something to consider is whether to remove the derive macro entirely, and blanket implement States.

@lee-orr lee-orr mentioned this pull request Sep 27, 2023
@alice-i-cecile
Copy link
Member

Something to consider is whether to remove the derive macro entirely, and blanket implement States.

I don't think we should, at least not in this PR. The explicitness of the trait has its upsides and this PR is not currently controversial.

@jnhyatt
Copy link
Contributor

jnhyatt commented Sep 28, 2023

I agree we keep the derive macro, at least for now. I suppose there's no real reason anymore since States impl is empty.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 28, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 28, 2023
@jnhyatt
Copy link
Contributor

jnhyatt commented Sep 29, 2023

I'm going to link #9957 here because it's something we'll want to look into merging once this is in -- it adds the ability to match on state patterns in run conditions and transition schedules. Definitely something we want if we're allowing "anything can be a state"

@lee-orr
Copy link
Contributor

lee-orr commented Sep 29, 2023

I do want to add that I think having the derive is very useful - for a couple of reasons:

a) even with all the changes here and in my PR, states still need to be Eq and discreet in some way. Even if theres infinite* options, you have to be able to say "were in the same state we were in last time".
b) the convention across bevy is to have a derive for different categories of types that we support - components, resources, etc. removing that for states will feel arbitrary in a way, and make it harder to tell when something can or cannot be considered a state

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 30, 2023
Merged via the queue into bevyengine:main with commit 9c00443 Sep 30, 2023
26 checks passed
@ItsDoot ItsDoot deleted the feat/simplify-states branch October 1, 2023 20:39
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
…evyengine#9945)

# Objective

The `States::variants` method was once used to construct `OnExit` and
`OnEnter` schedules for every possible value of a given `States` type.
[Since the switch to lazily initialized
schedules](https://github.com/bevyengine/bevy/pull/8028/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97f),
we no longer need to track every possible value.

This also opens the door to `States` types that aren't enums.

## Solution

- Remove the unused `States::variants` method and its associated type.
- Remove the enum-only restriction on derived States types.

---

## Changelog

- Removed `States::variants` and its associated type.
- Derived `States` can now be datatypes other than enums.

## Migration Guide

- `States::variants` no longer exists. If you relied on this function,
consider using a library that provides enum iterators.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
…evyengine#9945)

# Objective

The `States::variants` method was once used to construct `OnExit` and
`OnEnter` schedules for every possible value of a given `States` type.
[Since the switch to lazily initialized
schedules](https://github.com/bevyengine/bevy/pull/8028/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97f),
we no longer need to track every possible value.

This also opens the door to `States` types that aren't enums.

## Solution

- Remove the unused `States::variants` method and its associated type.
- Remove the enum-only restriction on derived States types.

---

## Changelog

- Removed `States::variants` and its associated type.
- Derived `States` can now be datatypes other than enums.

## Migration Guide

- `States::variants` no longer exists. If you relied on this function,
consider using a library that provides enum iterators.
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants