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

Operator for modifying the world while iterating #19

Open
Ralith opened this issue Dec 22, 2019 · 10 comments
Open

Operator for modifying the world while iterating #19

Ralith opened this issue Dec 22, 2019 · 10 comments
Labels
enhancement New feature or request

Comments

@Ralith
Copy link
Owner

Ralith commented Dec 22, 2019

Sometimes it's useful to allow components to be inserted/removed and entities to be spawned/despawned in the course of iteration. This is impossible using a conventional interface, but could be achieved with a sufficiently clever interface in the style of Vec::retain. For example:

fn modify<Q: Query, T>(
    &mut self,
    process: impl for<'a> FnMut(<Q::Fetch as Fetch<'a>>::Item) -> T,
    apply: impl FnMut(&mut ModifiableWorld, Entity, T),
)

where the two functions are called, one after the other, for each entity matching Q, and ModifiableWorld is a proxy object that adjusts the iteration state to account for entities being added/removed from the archetype currently being iterated.

The implementation is likely to be a source of significant complexity, and it does not enable anything you can't already do by making two passes and storing intermediate data in a Vec. That said, it would be neat.

@Ralith Ralith added the enhancement New feature or request label Dec 22, 2019
@cedric-h
Copy link
Contributor

cedric-h commented Dec 22, 2019

Some code samples that illustrate how a code sample would look with modify, using the workaround, and in specs where storages can be borrowed separately (at the cost of slow iteration)

// increment the plant timers,
// emit particles if they go up a stage.
// (amount of particles to emit is stored in growth stage)
world.modify::<&mut GrowthStage, _>(
    |growth_stage| {
        growth_stage.timer += 1;
        if growth_stage.timer >= growth_stage.duration {
            growth_stage.advance();
            Some(growth_stage.particle_count)
        } else {
            None
        }
    },
    |mut world, growing_ent, particle_count| {
        world.insert_one(growing_ent, ParticleEmitter {
            count: particle_count,
            .. ParticleEmitter::default()
        });
    }
);

/* versus:
 * how you have to do it now */

let needs_particles: Vec<_> = world.query::<&mut GrowthStage>()
    .iter()
    .filter_map(|(id, growth_stage)| {
        growth_stage.timer += 1;
        if growth_stage.timer >= growth_stage.duration {
            growth_stage.advance();
            Some((id, growth_stage.particle_count))
        } else {
            None
        }
    })
    .collect();

for (growing_ent, particle_count) in needs_particles.iter() {
    world.insert_one(ent, ParticleEmitter {
        count: particle_count,
        .. ParticleEmitter::default()
    });
}

/* versus:
 * specs approximation */
let mut particle_emitters = world.fetch_mut::<ParticleEmitter>();
let mut growth_stages = world.fetch_mut::<GrowthStages>();

for (growing_ent, mut growth_stage) in (&*world.entities(), &mut growth_stages) {
    growth_stage.timer += 1;
    if growth_stage.timer >= growth_stage.duration {
        growth_stage.advance();
        particle_emitters.insert(
            growing_ent,
            ParticleEmitter {
                count: growth_stage.particle_count,
                .. ParticleEmitter::default()
            },
        );
    }
}

@1tgr
Copy link

1tgr commented Apr 26, 2020

it does not enable anything you can't already do by making two passes and storing intermediate data in a Vec

Can the framework implementation take shortcuts (in the way that safe user code can't) so that the intermediate Vec is skipped?

@Ralith
Copy link
Owner Author

Ralith commented Apr 26, 2020

That's the theory. Unfortunately, as discussed in #20, the proposed interface runs afoul of rustc bugs regardless of implementation.

@Ralith
Copy link
Owner Author

Ralith commented May 20, 2020

New idea:

for mut entry in world.entries::<(&T, &U)>().iter() {
    let (t, u) = entry.get(); // uniquely borrows entry
    // ...
    if should_destroy {
        entry.remove(); // consumes entry by value, invalidating component references
    }
}

This could also support adding/removing individual components from the iterated entity. It wouldn't permit modifying/creating/destroying other entities, but it might still be quite useful, and I believe it avoids the rustc limitations we ran into with the original plan while also being considerably more ergonomic.

@Zireael07
Copy link

I am using hecs for a game, and I need to add/remove components while iterating the world or a query.

@Ralith
Copy link
Owner Author

Ralith commented Nov 9, 2020

For now, you should approach that by deferring the work until after the query, e.g. by collecting a list of components to add/remove in a Vec.

@BiosElement
Copy link

BiosElement commented Jan 9, 2022

Working on a roguelike movement system and have the following which seems a horrid mess. Anyone smarter than me have suggestions on a better way to do this? Basically querying for a component with an entity id and the desired position, then querying for the entity's current position and updating it, then finally deleting the WantsToMove component entity.

pub fn process_movement(world: &mut World) {
    println!("Processing movement boss!");
    let mut to_move = Vec::<MovingEntity>::new();
    for (id, wants_to_move) in &mut world.query::<&WantsToMove>() {
        to_move.push(MovingEntity {
            id: id,
            delta: Position {
                x: wants_to_move.x,
                y: wants_to_move.y,
            },
        });
    }

    for entity in &to_move {
        for (id, pos) in &mut world.query::<&mut Position>() {
            pos.x += entity.delta.x;
            pos.y += entity.delta.y;
        }
    }

    for entity in to_move {
        world.despawn(entity.id).unwrap();
    }
}

@Ralith
Copy link
Owner Author

Ralith commented Jan 9, 2022

I'm not sure what you're trying to do there; you're adding every single position delta to every single entity, then despawning every single entity that wanted to move?

@BiosElement
Copy link

@Ralith Apologies I wasn't as clear as I thought! I'm roughly working through HandsOnRust using hecs rather than legion and modeled it after the 'movement intents' system.

The current process is as follows.

Query for any entities which want to move which are created solely for this task. Then query for the entities that want to move to get their position and move them. Finally, despawn all the move request entities.

Thinking about it now, it seems a global vector containing movement requests would make more sense than using ECS currently, but I'd appreciate any suggestions as I'm still learning a lot. :)

@Ralith
Copy link
Owner Author

Ralith commented Jan 11, 2022

I'm not familiar with HandsOnRust, but that does sound like an unnecessarily complex and obscure solution to the hypothetical problem. I wouldn't recommend using the ECS to represent things other than logical entities; in general, hecs promotes using external data structures whenever that's more convenient. Storing requests in a vector on the side is likely to be the most convenient, because you can easily and efficiently populate it while iterating. You could later translate that back into components for the entities that want to move, but it would likely be simpler to consume directly.

Taken at face value, the most immediate issue I see with the above code is that you seem to be applying every WantsToMove to every Position, which seems unlikely to have been the intent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants