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

Ability to opt into change detection on *ReadOnlyItem in mutable QueryData derive #12920

Closed
Themayu opened this issue Apr 10, 2024 · 6 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature

Comments

@Themayu
Copy link
Contributor

Themayu commented Apr 10, 2024

What problem does this solve or what need does it fill?

Currently, the QueryData derive macro transforms fields of the type &'static mut T to Mut<'w, T> when generating the item type for the query. However, the readonly item type continues to use &'w T as the reference type. This makes it impossible to use change detection on the readonly form of mutable queries.

As an example from my code:

#[derive(QueryData)]
#[query_data(mutable)]
pub struct TextBoxQuery {
    pub data: &'static mut TextBox,
    pub style: &'static mut TextBoxStyle,
    pub text_style: &'static mut TextBoxTextStyle,
}

// derive(QueryData) output
pub struct TextBoxQueryItem<'w> {
    pub data: Mut<'w, TextBox>,
    pub style: Mut<'w, TextBoxStyle>,
    pub text_style: Mut<'w, TextBoxTextStyle>,
}

pub struct TextBoxQueryReadOnlyItem<'w> {
    pub data: &'w TextBox,
    pub style: &'w TextBoxStyle,
    pub text_style: &'w TextBoxTextStyle,
}

What solution would you like?

I would like a way to inform the QueryData derive macro to use bevy_ecs::change_detection::Ref<'w, T> instead of &'w T. I believe it would be useful if we can choose between applying it to the whole struct or to individual fields, like so:

Example 1: Whole struct application

#[derive(QueryData)]
#[query_data(mutable, detect_changes)]
pub struct TextBoxQuery {
    pub data: &'static mut TextBox,
    pub style: &'static mut TextBoxStyle,
    pub text_style: &'static mut TextBoxTextStyle,
}

// derive(QueryData) output
pub struct TextBoxQueryItem<'w> {
    pub data: Mut<'w, TextBox>,
    pub style: Mut<'w, TextBoxStyle>,
    pub text_style: Mut<'w, TextBoxTextStyle>,
}

pub struct TextBoxQueryReadOnlyItem<'w> {
    pub data: Ref<'w, TextBox>,
    pub style: Ref<'w, TextBoxStyle>,
    pub text_style: Ref<'w, TextBoxTextStyle>,
}

Example 2: Per field application

#[derive(QueryData)]
#[query_data(mutable)]
pub struct TextBoxQuery {
    #[query_data(detect_changes)]
    pub data: &'static mut TextBox,

    pub style: &'static mut TextBoxStyle,
    pub text_style: &'static mut TextBoxTextStyle,
}

// derive(QueryData) output
pub struct TextBoxQueryItem<'w> {
    pub data: Mut<'w, TextBox>,
    pub style: Mut<'w, TextBoxStyle>,
    pub text_style: Mut<'w, TextBoxTextStyle>,
}

pub struct TextBoxQueryReadOnlyItem<'w> {
    pub data: Ref<'w, TextBox>,
    pub style: &'w TextBoxStyle,
    pub text_style: &'w TextBoxTextStyle,
}

What alternative(s) have you considered?

The only other option I can see is for me to manually implement WorldQuery on TextBoxQuery, and I do not wish to manually implement WorldQuery due to how brittle it is.

@Themayu Themayu added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Apr 10, 2024
@Themayu
Copy link
Contributor Author

Themayu commented Apr 10, 2024

The detect_changes option name is up for discussion if necessary. I just chose the most obvious option because I didn't want to spend hours bikeshedding my own feature request before even posting it.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Apr 10, 2024
@alice-i-cecile
Copy link
Member

Seems like a very reasonable request. Mild preference for approach two, with per-field attributes since it's a bit more flexible.

@Themayu
Copy link
Contributor Author

Themayu commented Apr 10, 2024

@alice-i-cecile I think you misunderstood. There aren't two separate approaches here - I am proposing supporting both forms of usage, in a similar way to how serde has both #[serde(rename_all = ...)] at the type level and #[serde(rename = ...)] at the individual field level.

@alice-i-cecile
Copy link
Member

Ah, I'm fine with that as well.

@Themayu
Copy link
Contributor Author

Themayu commented May 11, 2024

I've come up with a (in my opinion) nicer way of achieving the goal here, which has been laid out in #13329.

@Themayu
Copy link
Contributor Author

Themayu commented May 14, 2024

Closed in favour of #13329.

@Themayu Themayu closed this as completed May 14, 2024
@Themayu Themayu closed this as not planned Won't fix, can't repro, duplicate, stale May 14, 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-Enhancement A new feature
Projects
None yet
Development

No branches or pull requests

2 participants