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

Add change detection that swaps between &/&mut and Ref/Mut when disabled/enabled #7344

Closed
wants to merge 7 commits into from

Conversation

Guvante
Copy link
Contributor

@Guvante Guvante commented Jan 23, 2023

Objective

  • Fixes Make Query<&T> return Ref<T>  #7322
  • When change detection is enabled &T becomes Ref and &mut T becomes Mut in queries
  • When change detection is disabled &T and &mut T stay as themselves

Solution


Changelog

  • Now you can disable change detection for a particular component which will elide the Ref/Mut types
#[derive(Component)]
#[component(change_detection = false)]
pub struct ChangeDetectionless;

ChangeDetectionless when queried will return &T and &mut T instead of a wrapper type and similarly change detection in general will be disabled for the type.

  • When change detection is enabled (the default) a wrapper type is returned that contains the change detection information for &T in a similar fashion to &mut T returning Mut<T> this type is Ref<T>

Migration Guide

  • All existing usages of a query containing &T will be returning Ref<T> unless change detection is disabled via the new change_detection(false) attribute on #[derive(Component)]. While many uses will work the same thanks to the Deref<Target = &T impl, pattern matching to get a value out will break. Either disable change detection if it is not required or avoid pattern matching and instead explicitly deref to get the underlying value.

@Guvante
Copy link
Contributor Author

Guvante commented Jan 23, 2023

Totally busted as I wanted to focus on the core concept here is a more concrete form than a comment. I did not fix all of the many pattern matches our unit tests do on &T queries which are now broken.

Additionally I didn't do any of the minute work that #6659 did like ensuring that a Ref/Mut attached to an disabled check would do the correct thing.

Future work includes adding an explicit Mut<T>, renaming RefFetch to ReadFetch, handling ZSTs correctly amongst a bunch of other things I forgot while putting this together.

Also I removed impl ChangeDetection for &mut T since that seemed like a bad idea. I plan to use the explicit Mut<T> wherever that would be required (since once I steal the NOP code from #6659 that should cover any cases that don't know what the type of T::WriteFetch` is.

Could in theory have been based off #6659 directly but my local environment happened to go haywire when I rebased/merged that PR so I started from scratch out of fear of something breaking. I can rejigger my history to point there if the credit needs to be more explicit.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR labels Jan 23, 2023
@alice-i-cecile
Copy link
Member

I really like the core concept! How would it work with runtime configured change detection?

@Guvante
Copy link
Contributor Author

Guvante commented Jan 23, 2023

You would return Ref/Mut and check the boolean flag in the requisite functions

@james7132
Copy link
Member

james7132 commented Jan 23, 2023

I actually don't really like the solution that #6659 currently has with the associated type, especially with how it impacts it's use in generic functions and systems. I've been meaning to update it, but haven't had the time. Would it be possible to just replace &T's WorldQuery::Item with Ref<T> for now?

@Guvante
Copy link
Contributor Author

Guvante commented Jan 24, 2023

Generic functions and systems could use Ref/Mut directly and avoid the problem. Not against just swapping &T and Ref<T> just doesn't sound like an interesting change to do the work for. Especially given there was a decent effort to go the opposite way in the past.

@Guvante
Copy link
Contributor Author

Guvante commented Jan 27, 2023

Well that was a lot. I believe this now has what work needs to be done to fix up all of the existing breakages from & -> Ref. Unfortunately I am not getting a ReadWrap doesn't implement Debug error in the custom_query_param.rs example and I am not sure why it needs to there... Especially when Mut doesn't implement Debug.

@Guvante Guvante force-pushed the ref_mut_on_change_detection branch from bc36996 to 9ec6820 Compare January 27, 2023 17:22
@Guvante
Copy link
Contributor Author

Guvante commented Jan 27, 2023

I actually don't really like the solution that #6659 currently has with the associated type, especially with how it impacts it's use in generic functions and systems. I've been meaning to update it, but haven't had the time. Would it be possible to just replace &T's WorldQuery::Item with Ref<T> for now?

Assuming generic code uses Ref/Mut explicitly I am not seeing anything in this PR that is any worse that Ref by itself would be. (The pain of not being able to pattern match on query results is non-trivial here)

Going to leave this open as a template for how Ref could work but unless opt-in change detection becomes the standard I think Ref isn't a good default given how much more verbose code becomes due to not being able to pattern match anymore.

Note of course that my changes were super rough so all of these might be solvable with some more finesse.

@Guvante Guvante marked this pull request as ready for review February 4, 2023 16:54
@Guvante Guvante force-pushed the ref_mut_on_change_detection branch from 2943174 to 2bb2ce7 Compare February 4, 2023 17:17
@james7132
Copy link
Member

Assuming generic code uses Ref/Mut explicitly I am not seeing anything in this PR that is any worse that Ref by itself would be. (The pain of not being able to pattern match on query results is non-trivial here)

The main issue here is that you cannot rely on Ref/Mut. If you use the associated type, you must use <T as Component>::AssociatedType, because it cannot be assumed that it's always Ref<T> and Mut<T>, which is very much not ergonomic in generic contexts.

@Guvante
Copy link
Contributor Author

Guvante commented Feb 6, 2023

I added support for using Ref/Mut as Query arguments which would work in generic contexts. Rather than saying Query<&T> you would say Query<Ref<T>>.

There may be some details that need to be done to ensure that the change detection deletion works properly there but it would be doable.

@cart
Copy link
Member

cart commented Apr 6, 2023

Well that was a lot. I believe this now has what work needs to be done to fix up all of the existing breakages from & -> Ref.

I'm really hesitant to make Ref<T> the default return type for change-detected &T. Wrapper types for references introduce serious ergonomics and complexity challenges to user code. The diffs for example/internal plugin code in this PR highlight how nasty this can be. We need that for Mut<T> to work, but foisting that complexity on users when it isn't absolutely necessary feels like a pretty big UX downgrade relative to the "consistency wins" of aligning the two.

@alice-i-cecile
Copy link
Member

I think this is a non-starter unless / until Rust has nicer support for smart pointer types in general.

rust-lang/rust#109939 is along those lines, so perhaps it will happen eventually.

@Guvante
Copy link
Contributor Author

Guvante commented Apr 6, 2023

This PR was mostly "I wonder how that will work" with a bit of "still an ergonomic improvement for non-tracked types".

Mostly I was curious how much code clutter it would form. Certainly this version is... Bad.

I am curious if I missed anything but don't think it is worth investigating so left it at this.

Certainly we can close this since there isn't an appetite.

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 D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Query<&T> return Ref<T>
4 participants