[Merged by Bors] - make ComponentTicks::set_changed public

[jakobhellermann]

fixes #1710

[mockersf]

for public consumption, could we have another method that takes a bool for set_changed?

[jakobhellermann]

It would also need to take the world for getting the current change tick, right? I haven't been following the reliable change detection work much.

[mockersf]

I didn't check either, but it looks like 0 or u32::MAX may be interesting values to set for "changed/didn't change"
never mind, as the value isn't reset after, it would be marked as changed for way too long

[alice-i-cecile]

My preferred API to expose for this would be a set_changed(bool) method on commands (and World) that operates on a single entity, following #1703 .

EDIT: At the same time, I think we should include a set_added(bool) method. Such functionality would be useful for similar reasons, and would be a nice bit of API consistency.

As a bit of context, @cart mentioned in #1471 that the requested functionality here might be needed, but to wait for a separate pr / use case. This seems like just that :)

[alice-i-cecile]

Adding to the 0.5 milestone: I'd like to avoid a regression in useful functionality, especially for such a critical ecosystem plugin.

[alice-i-cecile]

My concern with exposing the API directly as done in the first commit is that it's very, very coupled to the implementation details. This makes it very liable to break, and forces a huge burden of knowledge on the end user to use correctly.

[Davier]

I'm against set_changed(bool) and/or set_added(bool). It makes no sense to me to call them with false. What would be the use cases?

My concern with exposing the API directly as done in the first commit is that it's very, very coupled to the implementation details. This makes it very liable to break, and forces a huge burden of knowledge on the end user to use correctly.

ComponentTicks::set_changed is not something that should generally be used, but making it public for corner cases like described in #1710 seems ok to me. It could use a doc comment though. The alternative is to be forced to use ReflectMut (which is the intended way to do it, so I'd be ok with that too).

What cart suggested in #1471 is to have commands.set_changed::<T>(entity) to be able to set components as mutated without having mutable access to them, which is a different issue and doesn't solve "setting mutated for components of unknown types". I think that should be its own PR, and for reference my preferred solution is to add set_changed in ChangeTrackers<T> rather than a command.

[mockersf]

rather than having a set_changed(bool), what I would like to avoid is having a set_changed(u32) public as that may be very error prone if we let the user give whatever u32 they want...

[Davier]

rather than having a set_changed(bool), what I would like to avoid is having a set_changed(u32) public as that may be very error prone if we let the user give whatever u32 they want...

The issue as I understand it is that ReflectMut can't be used in bevy_inspector_egui because they have a custom reflection mechanism, so they want access to this low-level API instead. I'm not too worried about exposing it since you need to use some low-level unsafe API to even get a ComponentTicks struct, it's unlikely to be used by accident.

[alice-i-cecile]

I'm against set_changed(bool) and/or set_added(bool). It makes no sense to me to call them with false. What would be the use cases?

Hmm. I can imagine specialized use cases (e.g. specialized loading workflows) where we set these as false, but they would be both obscure and dangerous. Probably better to avoid encouraging it.

I'm not too worried about exposing it since you need to use some low-level unsafe API to even get a ComponentTicks struct, it's unlikely to be used by accident.

I can accept this, provided the doc strings are clear that it's probably not what you want to be using and we eventually create commands to set entities as added / changed in a user-friendly way.

[jakobhellermann]

I can accept this, provided the doc strings are clear that it's probably not what you want to be using and we eventually create commands to set entities as added / changed in a user-friendly way.

I added a doc comment explaining how this is usually done automatically.

[Davier]

Do you intend to fill this in this PR?

[jakobhellermann]

I just wanted the variables in scope, but I agree that todo!() doesn't really fit. Do you think changing it to unimplemented!() would be fine or would you like a larger example with proper values?

Its hard to come up with a simple example as this is the code I used (copied from bevy):

#[inline]
unsafe fn get_component_and_ticks(
    world: &World,
    component_id: ComponentId,
    entity: Entity,
    location: EntityLocation,
) -> Option<(*mut u8, *mut ComponentTicks)> {
    let archetype = &world.archetypes()[location.archetype_id];
    let component_info = world.components().get_info_unchecked(component_id);
    match component_info.storage_type() {
        StorageType::Table => {
            let table = &world.storages().tables[archetype.table_id()];
            let components = table.get_column(component_id)?;
            let table_row = archetype.entity_table_row(location.index);
            // SAFE: archetypes only store valid table_rows and the stored component type is T
            Some((
                components.get_unchecked(table_row),
                components.get_ticks_unchecked(table_row),
            ))
        }
        StorageType::SparseSet => world
            .storages()
            .sparse_sets
            .get(component_id)
            .and_then(|sparse_set| sparse_set.get_with_ticks(entity)),
    }
}

[cart]

Yeah lets just change these to unimplemented().

[jakobhellermann]

Done

[cart]

I'm cool with just making this public. ComponentTicks is "just" a holder of the last changed ticks. Adding a set_changed(bool) or set_changed() method feels like a higher level construct better suited to Mut pointers or something like ChangeTrackersMut..

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• markdownlint