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

[Merged by Bors] - Support tuple structs with #[derive(SystemParam)] #6957

Closed
wants to merge 1 commit into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Dec 14, 2022

Objective

Currently, only named structs can be used with the SystemParam derive macro.

Solution

Remove the restriction. Tuple structs and unit structs are now supported.


Changelog

  • Added support for tuple structs and unit structs to the SystemParam derive macro.

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 14, 2022

Note that this does not conflict with #6919.

@JoJoJet JoJoJet added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 14, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is correct and a natural extension. I don't think I'd ever use it myself though, and I'd complain against it being used in any project I was doing code review for.

Tuple structs are just generally kind of a mess to work with, and using them for reusable types like this feels wrong.

That said, if people want this I wouldn't want to block this PR.

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 15, 2022

I agree that tuple struct system params are not especially desirable -- named fields tend to be better (although this is true for tuple structs in general, it's not specific to SystemParams). Really, I see this PR as rounding out the engine by removing arbitrary limitations and places where users may experience friction. Given how prevalent anonymous tuples are in bevy, I believe most users will rightfully assume that tuple structs would just work with the derive macro. The moment when they try it and have to rewrite their type is an unfortunate toe-stub that I'd like to eliminate.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Gonna agree that this shouldn't be promoted, but it removes one sharp edge from the API.

@james7132 james7132 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 Dec 20, 2022
@cart
Copy link
Member

cart commented Dec 20, 2022

bors r+

bors bot pushed a commit that referenced this pull request Dec 20, 2022
# Objective

Currently, only named structs can be used with the `SystemParam` derive macro.

## Solution

Remove the restriction. Tuple structs and unit structs are now supported.

---

## Changelog

+ Added support for tuple structs and unit structs to the `SystemParam` derive macro.
@bors bors bot changed the title Support tuple structs with #[derive(SystemParam)] [Merged by Bors] - Support tuple structs with #[derive(SystemParam)] Dec 20, 2022
@bors bors bot closed this Dec 20, 2022
JoJoJet added a commit to JoJoJet/bevy that referenced this pull request Dec 21, 2022
Currently, only named structs can be used with the `SystemParam` derive macro.

Remove the restriction. Tuple structs and unit structs are now supported.

---

+ Added support for tuple structs and unit structs to the `SystemParam` derive macro.
@JoJoJet JoJoJet deleted the system-param-structs branch December 21, 2022 13:30
bors bot pushed a commit that referenced this pull request Dec 25, 2022
# Objective

* Currently, the `SystemParam` derive does not support types with const generic parameters.
  * If you try to use const generics, the error message is cryptic and unhelpful.
* Continuation of the work started in #6867 and #6957.

## Solution

Allow const generic parameters to be used with `#[derive(SystemParam)]`.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

Currently, only named structs can be used with the `SystemParam` derive macro.

## Solution

Remove the restriction. Tuple structs and unit structs are now supported.

---

## Changelog

+ Added support for tuple structs and unit structs to the `SystemParam` derive macro.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

* Currently, the `SystemParam` derive does not support types with const generic parameters.
  * If you try to use const generics, the error message is cryptic and unhelpful.
* Continuation of the work started in bevyengine#6867 and bevyengine#6957.

## Solution

Allow const generic parameters to be used with `#[derive(SystemParam)]`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Currently, only named structs can be used with the `SystemParam` derive macro.

## Solution

Remove the restriction. Tuple structs and unit structs are now supported.

---

## Changelog

+ Added support for tuple structs and unit structs to the `SystemParam` derive macro.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

* Currently, the `SystemParam` derive does not support types with const generic parameters.
  * If you try to use const generics, the error message is cryptic and unhelpful.
* Continuation of the work started in bevyengine#6867 and bevyengine#6957.

## Solution

Allow const generic parameters to be used with `#[derive(SystemParam)]`.
cart pushed a commit that referenced this pull request Apr 4, 2023
# Objective

The `#[derive(WorldQuery)]` macro currently only supports structs with
named fields.

Same motivation as #6957. Remove sharp edges from the derive macro, make
it just work more often.

## Solution

Support tuple structs.

---

## Changelog

+ Added support for tuple structs to the `#[derive(WorldQuery)]` macro.
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-Feature A new feature, making something new possible C-Usability A targeted 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