leudz / shipyard

Entity Component System focused on usability and flexibility.
Other
755 stars 46 forks source link

Component iteration without modification tracking #182

Closed parasyte closed 5 months ago

parasyte commented 1 year ago

Since 0e2d67ab125d83b800207e320afff7c6564a6a20, IntoIter now always yields Mut<T> for mutable components. In the latest published release, this was only true for components with tracking enabled, e.g. using the derive macro with #[track(Modification)].

There is one big problem with this new approach because field accesses to mutable components go through DerefMut which is not supported by two-phase borrows with operator traits like AddAssign. See:

Any component that has a struct field with a bitwise assignment operator implementation will have borrow check errors for code that normally wouldn't (without the DerefMut). An example using glam::Vec2 for its AddAssign impl:

use glam::f32::Vec2;
use shipyard::{Component, IntoIter as _, ViewMut, World};

#[derive(Component, Debug)]
struct Calc {
    vec: Vec2,
    scale: f32,
}

fn main() {
    let world = World::new();

    world.run(|mut calc: ViewMut<Calc>| {
        for mut calc in (&mut calc).iter() {
            calc.vec += calc.scale;
        }
    });
}

This works on the released version (0.6.2, with a warning about an unnecessary mutable variable), but fails on git HEAD:

error[E0502]: cannot borrow `calc` as immutable because it is also borrowed as mutable
  --> src\main.rs:15:25
   |
15 |             calc.vec += calc.scale;
   |             ------------^^^^------
   |             |           |
   |             |           immutable borrow occurs here
   |             mutable borrow occurs here
   |             mutable borrow later used here
   |
help: try adding a local storing this...
  --> src\main.rs:15:25
   |
15 |             calc.vec += calc.scale;
   |                         ^^^^^^^^^^
help: ...and then using that local here
  --> src\main.rs:15:13
   |
15 |             calc.vec += calc.scale;
   |             ^^^^^^^^^^^^^^^^^^^^^^

The workaround is trivial, just bind calc.scale to a temporary variable and add-assign with that on the right-hand side. But this is a pretty clear breaking change, and also a gnarly UX.

It would be nice to be able to get &mut Calc out of the iterator instead of Mut<Calc>. Both because it causes this issue, and because I don't need mutation tracking. That sounds like unnecessary overhead for default iteration behavior, doesn't it?

I am not sure what motivated the change, maybe https://github.com/leudz/shipyard/issues/157#issuecomment-1333155269 had something to do with it?

leudz commented 1 year ago

The main motivations are listed here: https://shipyard.zulipchat.com/#narrow/stream/269448-update/topic/On.20demand.20tracking/near/325747110. I agree it's not perfect, Mut is annoying and a cargo feature can't be added to solve the problem.

I think this is the third or forth iteration on the modification tracking design. So far they all come with some big problem but some of them can't even be worked around.

parasyte commented 1 year ago

Ah I didn't know there was a zulip. :eyes: I'll see you there!

leudz commented 5 months ago

The new design should fix this issue. https://github.com/leudz/shipyard/commit/eed63f6582c63da88dd7aed3784d7655a1970e34 This commit changes the type in iterators: https://github.com/leudz/shipyard/commit/d2f3c5a270f25026f11bd37a91436b7175a5e501