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

Make Query<&T> return Ref<T> #7322

Open
alice-i-cecile opened this issue Jan 22, 2023 · 4 comments
Open

Make Query<&T> return Ref<T> #7322

alice-i-cecile opened this issue Jan 22, 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

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 22, 2023

          A potentially more controversial option is just to have `&T` return `Ref<T>` instead. Though that might require some benchmarking to ensure there's no perf regression in doing so.

Originally posted by @james7132 in #7306 (comment)

A nice thing about making &T return Ref is that it would make the ReadOnly assoc type on &mut T have an "obvious answer", with this PR it could realistically be &T or Ref. I think the only downside that Mut has that Ref would also have is the fact that you cant pattern match on it, the other issues (shoving mut everywhere and borrowck annoyances from disjoint borrows) dont apply to Ref which is cool.

Boxy's followup.

This would align us further with the behavior of &mut, and ensure that change detection behavior is always available to users.

@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 X-Controversial There is active debate or serious implications around merging this PR labels Jan 22, 2023
@Guvante
Copy link
Contributor

Guvante commented Jan 23, 2023

There is also #6659 which is going in the opposite direction.

However on second thought both of these could be used together. Defaulting to change detection which returns Ref<T>/Mut<T> and if you disable change detection you get &T/&mut T could be pretty straightforward.

Also I don't know if it is good or bad but with simpler types it would be more common to disable change detection when it wasn't used.

@Guvante
Copy link
Contributor

Guvante commented Jan 23, 2023

It sounded interesting so I whipped together a first pass example of what I said. I can spend some time making the PR more thorough later if this path looks like an productive one.

@alice-i-cecile
Copy link
Member Author

Yeah, I rather like this set of changes: I'd be happy to see this proceed.

@cart
Copy link
Member

cart commented Apr 6, 2023

Left some thoughts here: #7344 (comment)

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

Successfully merging a pull request may close this issue.

3 participants