Encapsulate cfg(feature = "track_location") in a type.

[chescock]

Objective

Eliminate the need to write cfg(feature = "track_location") every time one uses an API that may use location tracking. It's verbose, and a little intimidating. And it requires code outside of bevy_ecs that wants to use location tracking needs to either unconditionally enable the feature, or include conditional compilation of its own. It would be good for users to be able to log locations when they are available without needing to add feature flags to their own crates.

Reduce the number of cases where code compiles with the track_location feature enabled, but not with it disabled, or vice versa. It can be hard to remember to test it both ways!

Remove the need to store a None in HookContext when the track_location feature is disabled.

Solution

Create an MaybeLocation<T> type that contains a T if the track_location feature is enabled, and is a ZST if it is not. The overall API is similar to Option, but whether the value is Some or None is set at compile time and is the same for all values.

Default T to &'static Location<'static>, since that is the most common case.

Remove all cfg(feature = "track_location") blocks outside of the implementation of that type, and instead call methods on it.

When track_location is disabled, MaybeLocation is a ZST and all methods are #[inline] and empty, so they should be entirely removed by the compiler. But the code will still be visible to the compiler and checked, so if it compiles with the feature disabled then it should also compile with it enabled, and vice versa.

Open Questions

Where should these types live? I put them in change_detection because that's where the existing MaybeLocation types were, but we now use these outside of change detection.

While I believe that the compiler should be able to remove all of these calls, I have not actually tested anything. If we want to take this approach, what testing is required to ensure it doesn't impact performance?

Migration Guide

Methods like Ref::changed_by() that return a &'static Location<'static> will now be available even when the track_location feature is disabled, but they will return a new MaybeLocation type. MaybeLocation wraps a &'static Location<'static> when the feature is enabled, and is a ZST when the feature is disabled.

Existing code that needs a &Location can call into_option().unwrap() to recover it. Many trait impls are forwarded, so if you only need Display then no changes will be necessary.

If that code was conditionally compiled, you may instead want to use the methods on MaybeLocation to remove the need for conditional compilation.

Code that constructs a Ref, Mut, Res, or ResMut will now need to provide location information unconditionally. If you are creating them from existing Bevy types, you can obtain a MaybeLocation from methods like Table::get_changed_by_slice_for() or ComponentSparseSet::get_with_ticks. Otherwise, you will need to store a MaybeLocation next to your data and use methods like as_ref() or as_mut() to obtain wrapped references.

[SpecificProtagonist]

Nice, we do want something like this.

I'll take a proper look later, but from a quick glance I can't see why we need TrackLocationOption<T>:
Shouldn't defining MaybeLocation as struct MaybeLocation { #[cfg(feature = "track_location")] caller: &'static Location<'static>} and using it in all the places where we previously had a cfg'd Location be enough? E.g. &'w mut MaybeLocation instead of TrackLocationOption<&'w mut &'static Location<'static>>.

Remove the need to store a None in HookContext when the track_location feature is disabled.

I was surprised that this didn't happen in the PR that introduced it (didn't get a chance to look at it before it got merged).

[chescock]

I'll take a proper look later, but from a quick glance I can't see why we need TrackLocationOption<T>:
Shouldn't defining MaybeLocation as struct MaybeLocation { #[cfg(feature = "track_location")] caller: &'static Location<'static>} and using it in all the places where we previously had a cfg'd Location be enough? E.g. &'w mut MaybeLocation instead of TrackLocationOption<&'w mut &'static Location<'static>>.

That would be simpler! I was trying to make sure that there was never any cost when the feature was disabled, though, and &mut ZST is pointer-sized instead of zero-sized. It would also mean things like set_spawned_or_despawned_by would have to actually do array indexing before assigning the ZST. I'm already a little worried that this PR will get rejected because we can't be certain that it's zero-cost :).

[Seldom-SE]

This would help avoid this pitfall I've seen a couple times cBournhonesque/lightyear#745 joseph-gio/bevy-trait-query#77

[eckz]

Just to avoid repetition in names like TrackLocationOption<Option<T>>, what about renaming TrackLocationOption to

pub struct MaybeLocation<T: ?Sized = &'static Location<'static>> { ... }

and removing the type alias

[chescock]

Just to avoid repetition in names like TrackLocationOption<Option<T>>, what about renaming TrackLocationOption to

pub struct MaybeLocation<T: ?Sized = &'static Location<'static>> { ... }

and removing the type alias

Hmm, that's tempting. I'm concerned that the default type there is a little complex, though; usually default types are simple things like () or Global. And the repetition isn't exactly wrong, as calling into_option() gives you an Option<Option<T>>.

I think I'll leave it alone for now, but if someone else chimes in with a preference to change it to that then I'm happy to do so!

[JaySpruce]

LGTM, but I do like eckz's suggestion. More complex than most default types maybe, but I think it'd be fine

[chescock]

This would help avoid this pitfall I've seen a couple times cBournhonesque/lightyear#745 joseph-gio/bevy-trait-query#77

Oh, I should add a note about Ref::new to the migration guide! I hadn't thought of that!

I also see that bevy-trait-query is using ? when creating Ref, which would be awkward to do with TrackLocationOption<Option<T>>. I'll add a transpose() function similar to Result::transpose() that will let you convert those to Option<TrackLocation<T>> to use with ?.

[chescock]

I do like eckz's suggestion. More complex than most default types maybe, but I think it'd be fine

Alright, two votes in favor sounds good to me! I'll make the change!