Require &mut self for World::increment_change_tick (#14459)
				
					
				
			# Objective
The method `World::increment_change_tick` currently takes `&self` as the
method receiver, which is semantically strange. Even though the interior
mutability is sound, the existence of this method is strange since we
tend to think of `&World` as being a read-only snapshot of a world, not
an aliasable reference to a world with mutability. For those purposes,
we have `UnsafeWorldCell`.
## Solution
Change the method signature to take `&mut self`. Use exclusive access to
remove the need for atomic adds, which makes the method slightly more
efficient. Redirect users to [`UnsafeWorldCell::increment_change_tick`]
if they need to increment the world's change tick from an aliased
context.
In practice I don't think there will be many breakages, if any. In cases
where you need to call `increment_change_tick`, you usually already have
either `&mut World` or `UnsafeWorldCell`.
---
## Migration Guide
The method `World::increment_change_tick` now requires `&mut self`
instead of `&self`. If you need to call this method but do not have
mutable access to the world, consider using
`world.as_unsafe_world_cell_readonly().increment_change_tick()`, which
does the same thing, but is less efficient than the method on `World`
due to requiring atomic synchronization.
```rust
fn my_system(world: &World) {
    // Before
    world.increment_change_tick();
    // After
    world.as_unsafe_world_cell_readonly().increment_change_tick();
}
```
			
			
This commit is contained in:
		
							parent
							
								
									8dc6ccfbe7
								
							
						
					
					
						commit
						218f78157d
					
				| @ -124,9 +124,7 @@ where | |||||||
|             let out = self.func.run(world, input, params); |             let out = self.func.run(world, input, params); | ||||||
| 
 | 
 | ||||||
|             world.flush(); |             world.flush(); | ||||||
|             let change_tick = world.change_tick.get_mut(); |             self.system_meta.last_run = world.increment_change_tick(); | ||||||
|             self.system_meta.last_run.set(*change_tick); |  | ||||||
|             *change_tick = change_tick.wrapping_add(1); |  | ||||||
| 
 | 
 | ||||||
|             out |             out | ||||||
|         }) |         }) | ||||||
|  | |||||||
| @ -2063,9 +2063,16 @@ impl World { | |||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
|     /// Increments the world's current change tick and returns the old value.
 |     /// Increments the world's current change tick and returns the old value.
 | ||||||
|  |     ///
 | ||||||
|  |     /// If you need to call this method, but do not have `&mut` access to the world,
 | ||||||
|  |     /// consider using [`as_unsafe_world_cell_readonly`](Self::as_unsafe_world_cell_readonly)
 | ||||||
|  |     /// to obtain an [`UnsafeWorldCell`] and calling [`increment_change_tick`](UnsafeWorldCell::increment_change_tick) on that.
 | ||||||
|  |     /// Note that this *can* be done in safe code, despite the name of the type.
 | ||||||
|     #[inline] |     #[inline] | ||||||
|     pub fn increment_change_tick(&self) -> Tick { |     pub fn increment_change_tick(&mut self) -> Tick { | ||||||
|         let prev_tick = self.change_tick.fetch_add(1, Ordering::AcqRel); |         let change_tick = self.change_tick.get_mut(); | ||||||
|  |         let prev_tick = *change_tick; | ||||||
|  |         *change_tick = change_tick.wrapping_add(1); | ||||||
|         Tick::new(prev_tick) |         Tick::new(prev_tick) | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
|  | |||||||
| @ -276,7 +276,10 @@ impl<'w> UnsafeWorldCell<'w> { | |||||||
|     pub fn increment_change_tick(self) -> Tick { |     pub fn increment_change_tick(self) -> Tick { | ||||||
|         // SAFETY:
 |         // SAFETY:
 | ||||||
|         // - we only access world metadata
 |         // - we only access world metadata
 | ||||||
|         unsafe { self.world_metadata() }.increment_change_tick() |         let change_tick = unsafe { &self.world_metadata().change_tick }; | ||||||
|  |         // NOTE: We can used a relaxed memory ordering here, since nothing
 | ||||||
|  |         // other than the atomic value itself is relying on atomic synchronization
 | ||||||
|  |         Tick::new(change_tick.fetch_add(1, std::sync::atomic::Ordering::Relaxed)) | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
|     /// Provides unchecked access to the internal data stores of the [`World`].
 |     /// Provides unchecked access to the internal data stores of the [`World`].
 | ||||||
|  | |||||||
		Loading…
	
		Reference in New Issue
	
	Block a user
	 Joseph
						Joseph