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

StarArawn · 2022-08-30T15:51:12Z

Objective

I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the last_change_tick is updated with the latest tick(from world). In my particular case I want to "wait" to update the last_change_tick until after my system runs for each entity.

Solution

Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose last_change_tick to the end users. This is achieved by adding the following to the System trait:

    /// Allows users to get the system's last change tick.
    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);

This causes a bit of weirdness with two implementors of System. FixedTimestep and ChainSystem both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems.

I think it's important to understand why I can't add the new functions only to the function system and not to the System trait. In my code I store the systems generically as Box<dyn System<...>>. I do this because I have differing parameters that are being passed in depending on the UI widget's system. As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are.

In my own code this ends up looking something like:

// Runs per entity.
let old_tick = widget_system.get_last_change_tick();
should_update_children = widget_system.run((widget_tree.clone(), entity.0), world);
widget_system.set_last_change_tick(old_tick);


// later on after all the entities have been processed:
for system in context.systems.values_mut() {
    system.set_last_change_tick(world.read_change_tick());
}

Changelog

Added get_last_change_tick and set_last_change_tick to System's.

StarArawn · 2022-08-30T15:57:54Z

Related to: #5633

cart · 2022-08-30T20:43:46Z

crates/bevy_ecs/src/system/system_chaining.rs

@@ -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 {


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.

cart · 2022-08-30T20:45:44Z

crates/bevy_time/src/fixed_timestep.rs

@@ -224,6 +224,15 @@ 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 {


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

alice-i-cecile · 2022-08-30T20:58:19Z

@DJMcNab, would this benefit from the removal of chained systems as a hardcoded object?

cart · 2022-08-30T21:17:12Z

@DJMcNab, would this benefit from the removal of chained systems as a hardcoded object?

I think it would just (effectively) implicitly do what @StarArawn did in the latest commit.

cart · 2022-08-30T21:24:17Z

crates/bevy_ecs/src/system/system.rs

+    /// Allows users to get the system's last change tick.
+    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);


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.

cart · 2022-08-30T21:25:15Z

crates/bevy_ecs/src/system/system.rs

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


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

DJMcNab · 2022-08-30T21:30:55Z

So the context here is running the same system multiple times within a single 'logical step'? I think in terms of chain system, this should be equivalent. I'm a little bit unsure why it can't just fall call the child's versions. Making chain system be a normal system would fix that. I've just now tried to resolve chain system, and it's still blocked on TAIT (.after breaks just using a naïve Box<dyn System>).

I'm tempted to say that the correct fix here is to make systems not track their own change tick at all, and always take it as an argument. That is, make the semantics be run_between_change_ticks. That being said, manual handling of change ticks in general seems like a very easy thing to get wrong, and I'm hesitant to support it at all.

Then again, we don't support use case cases where people run systems on their own outside of schedules, so if people want to have their fun, why not let them.
I'm hesistant to forward this guarantee onwards. That is, I'm happy enough if we merge this, but I don't want us to track this regressing somehow (e.g. a SystemParam can still assume that change ticks behave 'as-if' they were called from a schedule, with all the consequences thereof)

StarArawn · 2022-08-30T23:36:04Z

I'm tempted to say that the correct fix here is to make systems not track their own change tick at all, and always take it as an argument. That is, make the semantics be run_between_change_ticks. That being said, manual handling of change ticks in general seems like a very easy thing to get wrong, and I'm hesitant to support it at all.

This was the first idea I had however it felt a bit more breaking and I wanted to limit my scope here as much as possible.

cart · 2022-08-31T01:52:58Z

bors r+

) # Objective I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the `last_change_tick` is updated with the latest tick(from world). In my particular case I want to "wait" to update the `last_change_tick` until after my system runs for each entity. ## Solution Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose `last_change_tick` to the end users. This is achieved by adding the following to the `System` trait: ```rust /// Allows users to get the system's last change tick. 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); ``` This causes a bit of weirdness with two implementors of `System`. `FixedTimestep` and `ChainSystem` both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems. I think it's important to understand why I can't add the new functions only to the function system and not to the `System` trait. In my code I store the systems generically as `Box<dyn System<...>>`. I do this because I have differing parameters that are being passed in depending on the UI widget's system. As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are. In my own code this ends up looking something like: ```rust // Runs per entity. let old_tick = widget_system.get_last_change_tick(); should_update_children = widget_system.run((widget_tree.clone(), entity.0), world); widget_system.set_last_change_tick(old_tick); // later on after all the entities have been processed: for system in context.systems.values_mut() { system.set_last_change_tick(world.read_change_tick()); } ``` ## Changelog - Added `get_last_change_tick` and `set_last_change_tick` to `System`'s.

bors · 2022-08-31T02:08:33Z

Pull request successfully merged into main.

Build succeeded:

…vyengine#5838) # Objective I'm build a UI system for bevy. In this UI system there is a concept of a system per UI entity. I had an issue where change detection wasn't working how I would expect and it's because when a function system is ran the `last_change_tick` is updated with the latest tick(from world). In my particular case I want to "wait" to update the `last_change_tick` until after my system runs for each entity. ## Solution Initially I thought bypassing the change detection all together would be a good fix, but on talking to some users in discord a simpler fix is to just expose `last_change_tick` to the end users. This is achieved by adding the following to the `System` trait: ```rust /// Allows users to get the system's last change tick. 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); ``` This causes a bit of weirdness with two implementors of `System`. `FixedTimestep` and `ChainSystem` both implement system and thus it's required that some sort of implementation be given for the new functions. I solved this by outputting a warning and not doing anything for these systems. I think it's important to understand why I can't add the new functions only to the function system and not to the `System` trait. In my code I store the systems generically as `Box<dyn System<...>>`. I do this because I have differing parameters that are being passed in depending on the UI widget's system. As far as I can tell there isn't a way to take a system trait and cast it into a specific type without knowing what those parameters are. In my own code this ends up looking something like: ```rust // Runs per entity. let old_tick = widget_system.get_last_change_tick(); should_update_children = widget_system.run((widget_tree.clone(), entity.0), world); widget_system.set_last_change_tick(old_tick); // later on after all the entities have been processed: for system in context.systems.values_mut() { system.set_last_change_tick(world.read_change_tick()); } ``` ## Changelog - Added `get_last_change_tick` and `set_last_change_tick` to `System`'s.

StarArawn added 3 commits August 30, 2022 11:39

Added the ability to get or set the last change tick of a system.

0d385d5

Removed whitespace

fd4132f

Use implicit returns.

06a73ed

cart reviewed Aug 30, 2022

View reviewed changes

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Aug 30, 2022

alice-i-cecile requested a review from DJMcNab August 30, 2022 20:57

Added proper support for chained and fixed timestep systems.

e187d70

StarArawn requested a review from cart August 30, 2022 21:12

cart reviewed Aug 30, 2022

View reviewed changes

Changed doc comments.

e7eb188

StarArawn requested a review from cart August 30, 2022 23:36

Removed unused imports.

2d88809

bors bot changed the title ~~Added the ability to get or set the last change tick of a system.~~ [Merged by Bors] - Added the ability to get or set the last change tick of a system. Aug 31, 2022

bors bot closed this Aug 31, 2022

This was referenced Sep 1, 2022

Systems that can run multiple times that internally reset change detection #5851

Open

[Merged by Bors] - Add a change detection bypass and manual control over change ticks #5635

Closed

alice-i-cecile mentioned this pull request Nov 29, 2023

Remove System::{get,set}_last_run #10795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

StarArawn commented Aug 30, 2022

StarArawn commented Aug 30, 2022

cart Aug 30, 2022

cart Aug 30, 2022

alice-i-cecile commented Aug 30, 2022

cart commented Aug 30, 2022

cart Aug 30, 2022

cart Aug 30, 2022

DJMcNab commented Aug 30, 2022 •

edited

StarArawn commented Aug 30, 2022

cart commented Aug 31, 2022

bors bot commented Aug 31, 2022

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

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

Conversation

StarArawn commented Aug 30, 2022

Objective

Solution

Changelog

StarArawn commented Aug 30, 2022

cart Aug 30, 2022

Choose a reason for hiding this comment

cart Aug 30, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Aug 30, 2022

cart commented Aug 30, 2022

cart Aug 30, 2022

Choose a reason for hiding this comment

cart Aug 30, 2022

Choose a reason for hiding this comment

DJMcNab commented Aug 30, 2022 • edited

StarArawn commented Aug 30, 2022

cart commented Aug 31, 2022

bors bot commented Aug 31, 2022

DJMcNab commented Aug 30, 2022 •

edited