Skip to content

Conversation

@chescock
Copy link
Contributor

@chescock chescock commented Oct 3, 2024

Objective

Support accessing resources using reflection when using FilteredResources in a dynamic system. This is similar to how components can be queried using reflection when using FilteredEntityRef|Mut.

Solution

Change ReflectResource from taking &World and &mut World to taking impl Into<FilteredResources> and impl Into<FilteredResourcesMut>, similar to how ReflectComponent takes impl Into<FilteredEntityRef> and impl Into<FilteredEntityMut>. There are From impls that ensure code passing &World and &mut World continues to work as before.

Migration Guide

If you are manually creating a ReflectComponentFns struct, the reflect function now takes FilteredResources instead &World, and there is a new reflect_mut function that takes FilteredResourcesMut.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes labels Oct 3, 2024
@chescock chescock added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Blocked This cannot move forward until something else changes labels Oct 3, 2024
@chescock chescock marked this pull request as ready for review October 3, 2024 23:19
@BenjaminBrienen BenjaminBrienen added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 13, 2024
@BenjaminBrienen BenjaminBrienen added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 22, 2025
@chescock chescock added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 22, 2025
Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm not super confident in my ability to judge this code.

/// Function pointer implementing [`ReflectResource::reflect()`].
pub reflect: fn(&World) -> Option<&dyn Reflect>,
pub reflect: for<'w> fn(FilteredResources<'w, '_>) -> Option<&'w dyn Reflect>,
/// Function pointer implementing [`ReflectResource::reflect_mut()`].
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool if this could be a static assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure what you want asserted? I was trying to copy the style of the existing comments, and I think that's meant to be read as "ReflectResource::reflect_mut() invokes this function pointer".

…rces-ReflectResource

# Conflicts:
#	crates/bevy_ecs/src/system/builder.rs
Copy link
Contributor

@cBournhonesque cBournhonesque left a comment

Choose a reason for hiding this comment

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

Looks great!

@chescock chescock added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 14, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 16, 2025
@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Feb 16, 2025
Merged via the queue into bevyengine:main with commit 0a32450 Feb 16, 2025
31 checks passed
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-Straightforward Simple bug fixes and API improvements, docs, test and examples M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants