overload add / remove/ set component to operate on an array of entities too

[mikecann]

This is just a minor Quality of Life improvement..

It would be nice if addComponent, removeComponent and setComponent of world were able to operate on an array of entities in addition to just a single one.

I noticed that these functions return a boolean to indicate if the operation was successful or not.

https://github.com/hmans/miniplex/blob/changeset-release%2Fnext/packages/miniplex-core/src/World.ts#L63

If you supply an array of entities it would have to return an array of booleans.

[mikecann]

Actually, it would be nice if you could also pass an archetype to these functions too.

This is what I have been doing:

safeRemoveComponents(
    from: RegisteredEntity<Entity> | RegisteredEntity<Entity>[] | Archetype<any>,
    ...components: ComponentNames[]
  ) {
    const entities = `entities` in from ? from.entities : Array.isArray(from) ? from : [from];
    for (const entity of entities)
      for (const component of components)
        if (entity[component]) this.removeComponent(entity, component);
  }

There is probably a more performant way to do this without allocations and whatnot.

Also its might better to do this using Typescript function overloads if you want to change the return type depending on the input type

[hmans]

Hi Mike, thanks for opening the issue.

May I ask for some examples where this would provide a benefit over just iterating/mapping over an array (or archetype) in userland?

[mikecann]

Its purely a convenience, so instead of writing something like this:

const myArch = world.archetype("transform", "defaultAttackable", "cellModel");

  for(const entity of myArch.entities)
    world.removeComponent(entity, "defaultAttackable");

or

const myArch = world.archetype("transform", "defaultAttackable", "cellModel");
  myArch.entities.forEach(e => world.removeComponent(e, "defaultAttackable"));

I can write it like this:

  const myArch = world.archetype("transform", "defaultAttackable", "cellModel");
  world.removeComponent(myArch, "defaultAttackable");
  // or
  world.removeComponent(myArch.entities, "defaultAttackable");

It just makes it cleaner and means I dont have to worry about looping arrays backwards or anything like that which might be a concern with changing an array while iterating it.

As for examples, well I am using it in quite a few places. For example I have extended World to store some commonly accessed archetypes and helper functions:

export class GameWorld extends World<Entity> {
  public readonly cells = this.archetype("cellModel", "transform");
  public readonly ships = this.archetype("shipModel", "transform");
  public readonly boards = this.archetype("board", "transform");
  public readonly battles = this.archetype("battle");
  public readonly lights = this.archetype("light");
  public readonly eventReplays = this.archetype("eventReplay");
  public readonly cameraShakes = this.archetype("cameraShake", "age");

  public isDestroyed = false;

  findBoard = (playerId: string): EntityWith<"board" | "transform"> | undefined =>
    this.boards.entities.find((e) => e.board?.owner.id == playerId);

  getBoard = (playerId: string) =>
    ensure(this.findBoard(playerId), `Could not get board for player '${playerId}'`);

  findShip = (shipId: string): EntityWith<"shipModel" | "transform"> | undefined =>
    this.ships.entities.find((e) => e.shipModel.id == shipId);

  getShip = (shipId: string) => ensure(this.findShip(shipId), `Could not get ship '${shipId}'`);

  getCell = (address: CellAddress): EntityWith<"cellModel" | "transform"> =>
    this.getBoard(address.player).board.getCell(address.position);

  get battle(): EntityWith<"battle">["battle"] {
    return ensure(this.battles.first).battle;
  }

...

Then as the game is being played there are quite a number of places I do things like this:

  world.safeRemoveComponents(
    world.cells,
    "defaultAttackable",
    "pick",
    "pointerOut",
    "pointerOver",
    "nonTargetable",
    "targeted"
  );

Hope this helps to explain.

Not a big deal if its a headache to implement, just thought it might be a nice bit of sugar.

[hmans]

Alright, I'm seeing multiple concerns here that I think should be addressed separately. Since I'm in the middle of Miniplex 2.0 development (with a goal to push a first Beta this week!), I will look at these from 2.0's perspective. I hope you don't mind!

Quickly operating on an entire list of entities:

I'll give this some thought. The idea of supporting arrays of entities (and returning arrays of success/failure booleans) does not sit well with me at all, since it'd be a major complication of the API (including its typings) for what I feel is just a very minor convenience in userland.

I could imagine allowing the user to pass in an archetype (or, in Miniplex 2.0 lingo, a bucket) that the function will then operate on, which still leaves us with the question of the return value. I very certainly don't want to create an array with potentially hundreds (thousands? etc.) of values to return.

I think all in all I would highly prefer to make these sort of mass-operations more palatable in userland.

Fun fact: in 2.0, if you iterate over a bucket, the iteration already is safely performed "in reverse", ie. you can just do this:

for (const enemy of enemies)
  world.remove(enemy)

I'm looking into adding the usual iterator functions like .map and .forEach to buckets so this could also be written like this:

enemies.forEach(world.remove)

Or for removing a component and getting an array of result values

const success = enemies.map((e) => world.removeComponent(e, "health"))

Removing multiple components at once:

This is a good suggestion, and I will add a removeComponents(entity, ...components) function, or change the existing removeComponent function's signature to support more than one component.

The need to safely remove a component without Miniplex throwing an error:

This is already "fixed" in Miniplex 2.0. When you attempt to remove a component that isn't set, the function simply no-ops.

Finally, a question!

I've been wondering if returning true or false on success/failure of these operations even is needed. In your project, do you ever check them? Theoretically, if I changed all these functions to just return void and continue to no-op when no work can be done, we don't need to think about return values. ;-)

[mikecann]

I've been wondering if returning true or false on success/failure of these operations even is needed. In your project, do you ever check them? Theoretically, if I changed all these functions to just return void and continue to no-op when no work can be done, we don't need to think about return values. ;-)

Ye I was wondering about this. I certainty dont use the return value, I was assuming you had them there for a reason however hence my comments about returning an array if you pass in an array.

enemies.forEach(world.remove)

Unfortunately I dont think this would work due to binding semantics in JS.. would have to do this I suspect: enemies.forEach(world.remove.bind(world)) I suspect unless world.remove was an arrow function property on world rather than a function.

Fun fact: in 2.0, if you iterate over a bucket, the iteration already is safely performed "in reverse", ie. you can just do this:

Oh nice! Well that saves me some worries :)

Thanks again for taking the time for your detailed reply.

[hmans]

A quick update: addComponent and removeComponent now no longer return a success/failure value.

Unfortunately I dont think this would work due to binding semantics in JS.. would have to do this I suspect: enemies.forEach(world.remove.bind(world)) I suspect unless world.remove was an arrow function property on world rather than a function.

I've changed my new Bucket implementation to bind these to itself in its constructor, and also taught it a forEach function, so this would work now.

[hmans]

Hey Mike, I'd like to close this issue if that's okay with you. I've played with some potential implementations for these, but none of them gave me the feeling that the added complexity (especially in terms of typings) was justified by the value provided. If you disagree strongly, please let me know and I'll have another stab at it, but for now I feel this should be left to userland.

[mikecann]

Okay no worries, I appreciate you taking the time to consider it. If I still think its warranted once 2.0 comes out I might open another issue or attempt a solution myself in a PR

[hmans]

Hey @mikecann, I'm doing more with iterators now, which opens some charming new options for mass-changes/removals/etc. - first PR is linked to above, more are coming!

[mikecann]

Oh sweet! That looks awesome, nice work :)

[hmans]

You're going to hate me for this, but these changes are causing a massive design issue in a corner of this project that I did not expect: since I'm now checking the first argument if it's an iterable, it means that entities must not be iterables. This goes against "entities are just any JS object, no matter what it is". (And yes, it's not just a theoretical problem... it immediately broke some code of mine where I had buckets storing references to other buckets.)

So, unfortunately I have to pause this change again. But I promise I will revisit this at some point. 🙏

[mikecann]

Awww okay.. easy come easy go I guess!

What would you need to iterate on an entity for anyways?

[hmans]

Entities can be anything, so entities can themselves be buckets. Buckets are iterables. Bam, the new function implementations break.

[hmans]

Sorry, I didn't want to close this issue! I definitely want to revisit this at some point (but probably not before 2.0.)

[ExoticMatter]

Couldn't this be implemented as a separate set of functions? Then you wouldn't need to worry about overload behavior.

const myArch = world.archetype("transform", "defaultAttackable", "cellModel");
world.multiRemoveComponent(myArch.entities, "defaultAttackable");

[hmans]

Couldn't this be implemented as a separate set of functions? Then you wouldn't need to worry about overload behavior.

Yeah, it could. I've only been hesitant because this would essentially double the API surface only to save the user from writing a loop. I think this is fine:

for (const e of foo) world.removeComponent(e, "health")

But maybe that's just me. ;-) I'll think about it.