Skip to content

add support for non-'a lifetimes when deriving SystemParam#2633

Closed
ahouts wants to merge 5 commits intobevyengine:mainfrom
ahouts:support-non-a-lifetimes-when-deriving-SystemParam
Closed

add support for non-'a lifetimes when deriving SystemParam#2633
ahouts wants to merge 5 commits intobevyengine:mainfrom
ahouts:support-non-a-lifetimes-when-deriving-SystemParam

Conversation

@ahouts
Copy link

@ahouts ahouts commented Aug 11, 2021

Objective

Fixes #2587

Solution

Check if the struct definition contains a lifetime. If it does, use that instead of 'a.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 11, 2021
…mes-when-deriving-SystemParam

# Conflicts:
#	crates/bevy_ecs/macros/src/lib.rs
@alice-i-cecile
Copy link
Member

Hey, thanks for fixing this! It was a very silly little bug.

Co-authored-by: Garett Cooper <22463490+GarettCooper@users.noreply.github.com>
@mockersf mockersf added A-ECS Entities, components, systems, and events 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-Triage This issue needs to be labelled labels Aug 11, 2021
@mockersf
Copy link
Member

Maybe not compatible with #2605

@alice-i-cecile
Copy link
Member

IMO we should merge this ASAP, then rebase #2605 onto it. This change is much simpler, and I tend to find that order is much easier to fix things in.

@cart
Copy link
Member

cart commented Aug 12, 2021

I think first we should have a conversation about how deriving will work with the new lifetimes. I can think of a couple of paths forward:

  1. Use lifetime orders to map the x and y from struct MyParam<'x, 'y> to SystemParam<'w, 's>. If only one lifetime exists, assume it is w. If you have a struct that only needs an s lifetime, you must define both and then declare PhantomData for w.
  2. Only allow w and s lifetimes. Detect w-only, s-only, and w + s cases (to avoid the need for PhantomData) and handle them appropriately. Hard-code w and s in the SystemParam trait impl.

I'm honestly more in favor of (2). It requires using specific lifetimes, but it doesn't have weird implicit ordering requirements and seems more flexible / compatible with other non-w/s lifetimes. No matter what, making this work nicely is going outside the bounds of normal expectations. We'll need to make compromises somewhere.

@alice-i-cecile
Copy link
Member

I'm happy with 2 if the behavior is explicitly documented :)

@ahouts
Copy link
Author

ahouts commented Aug 13, 2021

Another option is to require an annotation on each lifetime. It would be more verbose, but at least it doesn't depend on seemingly innocuous things like lifetime name or ordering.

#[derive(SystemParam)]
struct MyParam<#[bevy_ecs(world)] 'w, #[bevy_ecs(state)] 's]> {
    foo: Res<'w, usize>,
    bar: Res<'s, usize>,
}

Or if you don't mind adding annotations that might conflict with other macros:

#[derive(SystemParam)]
struct MyParam<#[world] 'w, #[state] 's]> {
    foo: Res<'w, usize>,
    bar: Res<'s, usize>,
}

@ahouts
Copy link
Author

ahouts commented Aug 18, 2021

Regardless of what is chosen, this PR as written is no longer applicable.

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 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.

#[derive(SystemParam)] only works with lifetime named exactly 'a

5 participants