bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.94k stars 3.55k forks source link

Expose `SystemMeta` fields #5497

Open WinstonHartnett opened 2 years ago

WinstonHartnett commented 2 years ago

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

bevy_ecs' internal implementation of the default SystemParams use SystemMeta to initialize their states. Yet, only the is_send() field accessor is public to end-users and all other fields are pub(crate).

/// The metadata of a [`System`].
#[derive(Clone)]
pub struct SystemMeta {
    pub(crate) name: Cow<'static, str>,
    pub(crate) component_access_set: FilteredAccessSet<ComponentId>,
    pub(crate) archetype_component_access: Access<ArchetypeComponentId>,
    // NOTE: this must be kept private. making a SystemMeta non-send is irreversible to prevent
    // SystemParams from overriding each other
    is_send: bool,
    pub(crate) last_change_tick: u32,
}

Given that it's possible to directly implement SystemParam (and SystemParamFetch) for user-defined types in an unsafe impl, these fields should be exposed to advanced users.

What solution would you like?

Add unsafe mutable and safe immutable accessors for component_access_set and archetype_component_access, and immutable accessors for name and last_change_tick.

alice-i-cecile commented 2 years ago

I'm on board. Can you make a PR?

Very curious what your plans are here though.

WinstonHartnett commented 2 years ago

I'm on board. Can you make a PR?

Sure!

Very curious what your plans are here though.

I'm working on a deterministic entity ID allocator (in addition to Entity) for use in a delayed-lockstep multiplayer game. Because the simulation-related (i.e. deterministic) systems' scheduling orders are shared across clients, the simulation IDs are indexed by the system that spawned them (by hashing the corresponding SystemMeta's name). You can then use these deterministic IDs to map from one client's entities to another's, as Entity allocation is not deterministic.

DJMcNab commented 2 years ago

I'm not sure I want users to be implementing SystemParam - at the very least I'd be reluctant to claim it's currently supported.

I suppose unless/until we can work out a nicer API for dynamic components it's inevitable.

For your use case, we cannot provide the guarantee that the SystemMeta's name is unique per system as you seem to require it to be (since it's built on https://doc.rust-lang.org/std/any/fn.type_name.html) - the system name is absolutely not guaranteed to be the same across compilations too - e.g. for different platforms (or even within the same compilation, going by a strict reading of the docs). You need to do your own book-keeping.

E.g. with a Local initialised with the current system's id on first run (incrementing the ID counter in the progress), or a constant per system, etc.

alice-i-cecile commented 2 years ago

Ping @BoxyUwU and @TheRawMeatball for opinions here. I share @DJMcNab's concerns, but I also don't like how the trait is public (and not sealed) but not feasibly implementable.

WinstonHartnett commented 2 years ago

I'm not sure I want users to be implementing SystemParam - at the very least I'd be reluctant to claim it's currently supported.

I suppose unless/until we can work out a nicer API for dynamic components it's inevitable.

For your use case, I'm we cannot provide the guarantee that the SystemMeta's name is unique per system as you seem to require it to be (since it's built on https://doc.rust-lang.org/std/any/fn.type_name.html). You need to do your own book-keeping.

Yes that's unfortunate, but I've never run into problems [yet]. I'd prefer eventually using a stable TypeID (like #32). Nonetheless, it'd be nice to have the name and access information on-hand for debugging, although I'm sure you can get it from Schedule.

E.g. with a Local initialised with the current system's id on first run (incrementing the ID counter in the progress), or a constant per system, etc.

Yes, I see now that locals can be created with FromWorld, so I guess I don't need a custom SystemParam. Is the SystemParam initialization order stable across clients/binaries?

In the past, I've used a hash of a type's AST (along with the definition file location), but it's only marginally safer as a unique, binary-agnostic ID than type_name or type_id.

alice-i-cecile commented 2 years ago

Is the SystemParam initialization order stable across clients/binaries?

This should be, but I don't think we explicitly make any guarantees to this effect yet.

TheRawMeatball commented 2 years ago

I'd be OK merging such a change until we make a proper API for creating dynamic SystemParam s, though I still think the ideal version of such an api would involve

WinstonHartnett commented 1 year ago

Just ran into this again, so I pushed some changes in #7119 . Some comments/questions:

richchurcher commented 1 week ago

Potential adopters can find context in now-closed PR #7119.