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.

Sign up for GitHub

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

Jump to bottom

Add IterableEnum trait #7269

Closed
wants to merge 8 commits into from
Closed

Add IterableEnum trait #7269

wants to merge 8 commits into from

Conversation

DasLixou
Copy link
Contributor

Objective

Required from #7267

Solution

  • Add required things

Changelog

  • Add IterableEnum trait and derive
  • Add EnumIterator iterator

@DasLixou
Copy link
Contributor Author

One thing i want to resolve before this gets merged:
Currently, an enum MUST have only Unit enum fields (so without any values). But @alice-i-cecile said i should try to implement it on bigger bevy_input enums. now, there are some but mostly they'll be like GamepadButtonType and have an ending Other(u32) variant which makes it not possible. Now my question: should i make it possible and then just iterate over the Unit variants?

james7132
james7132 requested changes Jan 18, 2023
View reviewed changes
crates/bevy_utils/macros/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_utils/macros/Cargo.toml Outdated
[dependencies]
syn = { version = "1.0", features = ["full", "parsing", "extra-traits"] }
quote = "1.0"
bevy_macro_utils = { version = "0.9.0", path = "../../bevy_macro_utils" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really confusing to now have both bevy_macro_utils and bevy_utils_macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already thought about that on discord. But no answer from Alice yet

crates/bevy_utils/src/iterable_enum.rs Outdated Show resolved Hide resolved
crates/bevy_utils/macros/src/lib.rs Show resolved Hide resolved
crates/bevy_utils/macros/src/iterable_enum.rs

use crate::paths;

pub fn parse_iterable_enum_derive(input: TokenStream) -> TokenStream {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a get_at, could we not just generate a const SLICE: &[Self] in the trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty smart idea. I’ll try

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we lose the trait altogether and just generate an associated const on the type itself?

crates/bevy_utils/Cargo.toml Outdated
@@ -13,7 +13,8 @@ ahash = "0.7.0"
tracing = { version = "0.1", default-features = false, features = ["std"] }
instant = { version = "0.1", features = ["wasm-bindgen"] }
uuid = { version = "1.1", features = ["v4", "serde"] }
hashbrown = { version = "0.12", features = ["serde"] }
hashbrown = { version = "0.12.3", features = ["serde"] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
hashbrown = { version = "0.12.3", features = ["serde"] }
hashbrown = { version = "0.12", features = ["serde"] }

@DasLixou @james7132
Update crates/bevy_utils/macros/Cargo.toml
5bf736d 
Co-authored-by: James Liu <contact@jamessliu.com>
@james7132
Copy link
Member

One thing i want to resolve before this gets merged: Currently, an enum MUST have only Unit enum fields (so without any values). But @alice-i-cecile said i should try to implement it on bigger bevy_input enums. now, there are some but mostly they'll be like GamepadButtonType and have an ending Other(u32) variant which makes it not possible. Now my question: should i make it possible and then just iterate over the Unit variants?

We could add a #[enum(ignore)] for the variants that should be skipped.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events A-Utils Utility functions and types C-Enhancement A new feature labels Jan 18, 2023
@DasLixou @james7132
Update crates/bevy_utils/src/iterable_enum.rs
e29579c 
Co-authored-by: James Liu <contact@jamessliu.com>
@alice-i-cecile
Copy link
Member

We could add a #[enum(ignore)] for the variants that should be skipped.

Yes please.

@DasLixou @james7132
Update crates/bevy_utils/macros/src/lib.rs
51d0433 
Co-authored-by: James Liu <contact@jamessliu.com>
@alice-i-cecile alice-i-cecile mentioned this pull request Jan 18, 2023
Closed
33 tasks
@DasLixou
Copy link
Contributor Author

We could add a #[enum(ignore)] for the variants that should be skipped.

Yes please.

I’ll try to understand how it works for bundles and then try it 😁

@mockersf
Copy link
Member

Why not use an existing one? For example https://crates.io/crates/enum-iterator

@DasLixou
Copy link
Contributor Author

Why not use an existing one? For example https://crates.io/crates/enum-iterator

That would be too easy 🤧

@DasLixou
Copy link
Contributor Author

@alice-i-cecile what do you think? enum-iterator really looks... good 🥲

@mockersf
Copy link
Member

That would be too easy 🤧

Then I'm definitely against this PR, even more so as it adds a new crate

@DasLixou
Copy link
Contributor Author

That would be too easy 🤧

Then I'm definitely against this PR, even more so as it adds a new crate

Should i convert this PR to a import lib and put it in the input enums then?

@mockersf
Copy link
Member

Should i convert this PR to a import lib and put it in the input enums then?

I don't know, there are no details on why this macro is needed 🤷

@DasLixou
Copy link
Contributor Author

Should i convert this PR to a import lib and put it in the input enums then?

I don't know, there are no details on why this macro is needed 🤷

alice had it on her todos and i asked if i could do it. the macro was planned all the time. but a crate will be better yes. now should i just close the PR or migrate my "code" to the crate?

@mockersf
Copy link
Member

alice had it on her todos and i asked if i could do it. the macro was planned all the time. but a crate will be better yes. now should i just close the PR or migrate my "code" to the crate?

I must say I don't see the link between stageless schedule and being able to iterate on KeyCodes... so I don't know...

@DasLixou
Copy link
Contributor Author

alice had it on her todos and i asked if i could do it. the macro was planned all the time. but a crate will be better yes. now should i just close the PR or migrate my "code" to the crate?

I must say I don't see the link between stageless schedule and being able to iterate on KeyCodes... so I don't know...

alice just threw it out in the conversation

@DasLixou
Copy link
Contributor Author

Closing in favor of enum-iterator

@DasLixou DasLixou closed this Jan 18, 2023
@DasLixou DasLixou deleted the iterable-enum branch January 18, 2023 20:59
@alice-i-cecile
Copy link
Member

Alright, I'm on board. We can use enum-iterator :)

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

@JoJoJet JoJoJet JoJoJet left review comments

@james7132 james7132 james7132 requested changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Utils Utility functions and types C-Enhancement A new feature
Projects
Scheduling
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DasLixou @james7132 @alice-i-cecile @mockersf @JoJoJet