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

[Merged by Bors] - Added the ability to get or set the last change tick of a system. #5838

Closed
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,14 @@ where
out
}

fn get_last_change_tick(&self) -> u32 {
self.system_meta.last_change_tick
}

fn set_last_change_tick(&mut self, last_change_tick: u32) {
self.system_meta.last_change_tick = last_change_tick;
}

#[inline]
fn apply_buffers(&mut self, world: &mut World) {
let param_state = self.param_state.as_mut().expect(Self::PARAM_MESSAGE);
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub trait System: Send + Sync + 'static {
fn archetype_component_access(&self) -> &Access<ArchetypeComponentId>;
/// Returns true if the system is [`Send`].
fn is_send(&self) -> bool;

/// Runs the system with the given input in the world. Unlike [`System::run`], this function
/// takes a shared reference to [`World`] and may therefore break Rust's aliasing rules, making
/// it unsafe to call.
Expand Down Expand Up @@ -59,6 +60,10 @@ pub trait System: Send + Sync + 'static {
fn default_labels(&self) -> Vec<SystemLabelId> {
Vec::new()
}
/// Allows users to get the system's last change tick.
Copy link
Member

Choose a reason for hiding this comment

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

"Users" in this context is implicit. I think this should just say something like "Gets the system's last change tick". Same for the set method.

fn get_last_change_tick(&self) -> u32;
/// Allows users to set the system's last change tick.
fn set_last_change_tick(&mut self, last_change_tick: u32);
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in #5633, I think this deserves some level of warning around the effects this will have on change detection / that setting it manually could break things if you aren't careful.

}

/// A convenience type alias for a boxed [`System`] trait object.
Expand Down
11 changes: 11 additions & 0 deletions crates/bevy_ecs/src/system/system_chaining.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use bevy_utils::tracing::warn;

use crate::{
archetype::ArchetypeComponentId,
component::ComponentId,
Expand Down Expand Up @@ -106,6 +108,15 @@ impl<SystemA: System, SystemB: System<In = SystemA::Out>> System for ChainSystem
self.system_a.check_change_tick(change_tick);
self.system_b.check_change_tick(change_tick);
}

fn get_last_change_tick(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

I would argue that if we can't implement these functions for every System, they aren't System functions.
That being said, it seems like chained systems should just assume a shared change tick? At the very least, setting last_change_tick for both seems correct.

self.system_a.get_last_change_tick()
}

fn set_last_change_tick(&mut self, last_change_tick: u32) {
self.system_a.set_last_change_tick(last_change_tick);
self.system_b.set_last_change_tick(last_change_tick);
}
}

/// An extension trait providing the [`IntoChainSystem::chain`] method for convenient [`System`]
Expand Down
10 changes: 9 additions & 1 deletion crates/bevy_time/src/fixed_timestep.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bevy_ecs::{
system::{IntoSystem, Res, ResMut, Resource, System},
world::World,
};
use bevy_utils::HashMap;
use bevy_utils::{tracing::warn, HashMap};
use std::borrow::Cow;

/// The internal state of each [`FixedTimestep`].
Expand Down Expand Up @@ -224,6 +224,14 @@ impl System for FixedTimestep {
fn check_change_tick(&mut self, change_tick: u32) {
self.internal_system.check_change_tick(change_tick);
}

fn get_last_change_tick(&self) -> u32 {
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Why can't we just pipe through the internal system's last_change_tick?

self.internal_system.get_last_change_tick()
}

fn set_last_change_tick(&mut self, last_change_tick: u32) {
self.internal_system.set_last_change_tick(last_change_tick);
}
}

#[cfg(test)]
Expand Down