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

Expose method to update the internal ticks of Ref and Mut #17716

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cBournhonesque
Copy link
Contributor

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

There are some situations (#13735) where the ticks that are present inside Ref are incorrect, for example if Ref is created outside of a SystemParam.
I still want to use Ref because it has convenient is_added and is_changed methods.

My current solution is to build my own Ref by copy-pasting most the bevy code to do that via something like

/// This method is necessary because there is no easy way to 
pub(crate) fn get_ref<C: Component>(
    world: &World,
    entity: Entity,
    last_run: Tick,
    this_run: Tick,
) -> Ref<C> {
    unsafe {
        let component_id = world
            .components()
            .get_id(TypeId::of::<C>())
            .unwrap_unchecked();
        let world = world.as_unsafe_world_cell_readonly();
        let entity_cell = world.get_entity(entity).unwrap_unchecked();
        get_component_and_ticks(
            world,
            component_id,
            C::STORAGE_TYPE,
            entity,
            entity_cell.location(),
        )
        .map(|(value, cells, _caller)| {
            Ref::new(
                value.deref::<C>(),
                cells.added.deref(),
                cells.changed.deref(),
                last_run,
                this_run,
                #[cfg(feature = "track_location")]
                _caller.deref(),
            )
        })
        .unwrap_unchecked()
    }
}

// Utility function to return
#[inline]
unsafe fn get_component_and_ticks(
    world: UnsafeWorldCell<'_>,
    component_id: ComponentId,
    storage_type: StorageType,
    entity: Entity,
    location: EntityLocation,
) -> Option<(Ptr<'_>, TickCells<'_>, MaybeUnsafeCellLocation<'_>)> {
    match storage_type {
        StorageType::Table => {
            let table = unsafe { world.storages().tables.get(location.table_id) }?;

            // SAFETY: archetypes only store valid table_rows and caller ensure aliasing rules
            Some((
                table.get_component(component_id, location.table_row)?,
                TickCells {
                    added: table
                        .get_added_tick(component_id, location.table_row)
                        .unwrap_unchecked(),
                    changed: table
                        .get_changed_tick(component_id, location.table_row)
                        .unwrap_unchecked(),
                },
                #[cfg(feature = "track_location")]
                table
                    .get_changed_by(component_id, location.table_row)
                    .unwrap_unchecked(),
                #[cfg(not(feature = "track_location"))]
                (),
            ))
        }
        StorageType::SparseSet => {
            let storage = unsafe { world.storages() }.sparse_sets.get(component_id)?;
            storage.get_with_ticks(entity)
        }
    }
}

It would be very convenient if instead bevy exposed a way to create a Ref object with custom last_run and this_run ticks.
This PR does this by exposing a function to overwrite the last_run and this_run ticks.
(Same with Mut)

I am ok with marking the method unsafe or risky if it's deemed to risky for end-users.

@cBournhonesque cBournhonesque added A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Feb 7, 2025
Copy link
Contributor

@chescock chescock left a comment

Choose a reason for hiding this comment

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

Yeah, there are too many places in Bevy where we pull the ticks out of the World without any way to set them correctly from within a system. This seems like a good way to let users work around that issue!

I do think we could at least make Query<EntityRef> work automatically, but making it automatic for methods on &World will be tough.

It would be very convenient if instead bevy exposed a way to create a Ref object with custom last_run and this_run ticks.

My first thought was to note that Ref::new() lets you do that. But you can't get the added and changed ticks back out of an existing Ref, so there's still no way to map an existing Ref to one with different system ticks! (I assume you know this, but I'm writing it in case other reviewers have the same first reaction that I did.)

@cBournhonesque
Copy link
Contributor Author

My first thought was to note that Ref::new() lets you do that. But you can't get the added and changed ticks back out of an existing Ref, so there's still no way to map an existing Ref to one with different system ticks! (I assume you know this, but I'm writing it in case other reviewers have the same first reaction that I did.)

Exactly, exposing methods to get the added/changed ticks from an existing Ref would also work for me!

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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants