Skip to content

Comments

Allow synonyms for 'w and 's lifetimes#8235

Open
CGMossa wants to merge 8 commits intobevyengine:mainfrom
CGMossa:allow_synonyms_for_w_s
Open

Allow synonyms for 'w and 's lifetimes#8235
CGMossa wants to merge 8 commits intobevyengine:mainfrom
CGMossa:allow_synonyms_for_w_s

Conversation

@CGMossa
Copy link
Contributor

@CGMossa CGMossa commented Mar 28, 2023

Objective

The lifetimes 'w and 's are confusing.

Solution

One way to solve it by is by adding the ability to name the lifetimes different things depending on the context.
This was suggested by @maniwani here #8201 (comment)

Decide if there should be added synonyms for these traits / types:

  • Add the synonyms to SystemParam.
  • ParamSet?
  • Query?

Tasks:

CGMossa added 2 commits March 28, 2023 13:51
QUESTION: Where should this be located?
@CGMossa
Copy link
Contributor Author

CGMossa commented Mar 28, 2023

Note that this was brought up due to #8201. But I don't know if they should depend on each or whatever is the best decision.

@CGMossa
Copy link
Contributor Author

CGMossa commented Mar 28, 2023

One problem that I tried to solve, which may be beyond this PR, is that SystemParam-derive does not allow generics that aren't 'w and 's. Should it do that? And if so, can I have some help with that?

@joseph-gio joseph-gio self-requested a review March 28, 2023 14:18
@joseph-gio joseph-gio added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 28, 2023
@joseph-gio joseph-gio self-requested a review March 28, 2023 14:25
Copy link
Member

@joseph-gio joseph-gio left a comment

Choose a reason for hiding this comment

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

Thank you for implementing this, I've been meaning to get around to it for a while now.

One concern I have is that allowing so many different synonyms of 's might make the situation more confusing -- I think we should agree on one of the options, and then only allow that one name, or the first letter of it.

CGMossa and others added 2 commits March 28, 2023 21:05
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
Moved tests
simplified tests.
@CGMossa
Copy link
Contributor Author

CGMossa commented Mar 28, 2023

Sure! I'll leave that to other reviewers :)

@joseph-gio
Copy link
Member

joseph-gio commented Apr 18, 2023

After thinking about it a bit, my preference for the name of the 's lifetime is 'system. It just feels like the most clear and natural option to me. If you do that and fix up the merge conflicts, then I think this should be a pretty uncontroversial PR.

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.

Code quality looks good, but I would prefer not to introduce a proliferation of options. Sticking to 'w/'world and 's/ 'state seems like a healthy compromise here.

@janhohenheim janhohenheim added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 15, 2024
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 S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants