Entt: [Feature request] Get component if it exists or Assign

[ErikBehar]

Just adding here so it doesn't get lost:

We have:

//accommodate
if(registry.has<Comp>(entity)) {
    registry.replace<Comp>(entity, arg1, argN);
} else {
    registry.assign<Comp>(entity, arg1, argN);
}

Would like to add something similar that does:

 // some other word
if(registry.has<Comp>(entity)) {
    registry.get<Comp>(entity);
} else {
    registry.assign<Comp>(entity, arg1, argN);
}

[skypjack]

registry.get<Comp>(entity, arg1, argN);

Is it an error or do you expect that the get uses the arguments somehow?

[ErikBehar]

oops, yeah copy paste error ... no arguments for the .get needed, I've edited it.

[skypjack]

Ok, so, the main problem I see with this is that it's not clear what an user should expect when get is invoked instead of assign.
I mean, you pass arguments in both cases. Do you expect them to be discarded or used anyway to update the already existing instance? It could be misleading to discard them maybe.

[Kerndog73]

As @skypjack said, sometimes discarding arguments sounds a bit fishy to me. You could do this:

if (auto *comp = reg.get_if<Comp>(e)) {
  // Do stuff with comp
} else {
  Comp &comp = reg.assign<Comp>(e);
  // If you need to do stuff with comp here then
  // maybe you need a helper function
}

If there were to be a new helper function in EnTT, what would it be called? How best describe the behavior? You're getting and assigning so it kind of sounds like std::exchange but not quite.

[ErikBehar]

yeah I see its a bit vague ...

I'm not using the arguments myself, but I can see somebody using them. These helpers are great for shortening the code, and in my code I'm going to use the component right away, so I'd like it to be ready/initialized, so it makes sense the way its up there now.

if you are going to replace the insides of the component, might as well use accommodate, so it makes no sense to apply the arguments in the .get case, and it makes total sense to apply them in the assign case.

its nice to just say :
r.getOrAssign( a, b, c ).someVal *= r.getOrAssign( x, y, z).val

instead of the 8 lines or so of code you'd need otherwise.

[skypjack]

get_or_assign sounds good indeed. @Kerndog73 ? :smile:
The standard library has something similar after all.

---

Uhm, no, I was wrong, that's insert_or_assign. Actually, get_or_assign is really misleading, I cannot even imagine where the arguments go.
I think the worst thing is if destructing one of the arguments has a side effect, you cannot know if it's to be expected or not, therefore you cannot be prepared to manage it.

I'm not that sure this is a good idea @ErikBehar I'm sorry.
Someone has enough arguments to convince me?

[Kerndog73]

insert_or_assign always uses the given arguments and the object is left in the same state regardless of whether or not the key was already in the map. accommodate does this as well. But get_or_assign leaves the entity in a different state depending whether a component exists. insert_or_assign and accomodate are more well-behaved and deterministic.

As @skypjack said, destroying the arguments could have side effects like decrementing the reference count of a std::shared_ptr.

[ErikBehar]

alright so just drop the arguments. Now we know the state in both cases is clear.
of course now I just need more lines of code, in the case that I need to initialize to do the same thing.

[Kerndog73]

@ErikBehar There's nothing stopping you from implementing this function in your own code. I think @skypjack and I both agree that this doesn't belong in EnTT.

[ErikBehar]

@Kerndog73 I already implemented it before I brought it up in the chat ...
So whats the negative argument for the parameter less version ?

[Kerndog73]

By "parameter less version", do you mean get_or_default_construct?

If so, then this version isn't as bad because you aren't sometimes discarding arguments. Although, it still leaves the component in either a default constructed state or it's previous state depending on whether it exists.

My main argument against this is that it feels wrong.

Perhaps we should wait for a response from @skypjack. It's currently very late in his timezone so we'll probably hear from him in 7 or 8 hours.

[skypjack]

Yeah, sorry, I'm just on the other side of the world after all.
A sort of _get it no matter if it doesn't exist_ is less counterintuitive, but then I don't see the benefits for the client. I mean, you don't know if you have to update or not the returned component unless you invoke has and rely on an if, that was exactly what you were doing before.

---

Let's try to flip the problem on its head.
Why do you need this? What's exactly your problem that requires such a strange pattern?
Try to share our knowledge. It could happen that you find a different way to solve it and get rid of this annoying requirement.

[ErikBehar]

In my use case I'm creating links between entities by storing their entity IDs on components on each entity.

So I'm not really using the arguments. Looks similar to the example above with out the params.

At the end of the day all it does is save me writing out boiler plate 8 lines or calling a helper function that does if has. It's really just the same as accommodate, there's no reason whatsoever to have that, you can use if has same as above, but saves u some lines of code.

[skypjack]

The last proposal (the one without arguments) works similar to the operator[] of an std::map:

Returns a reference to the value that is mapped to a key equivalent to key, performing an insertion if such key does not already exist.

Replace _key_ with _entity identifier_ and that's it. So, it's in line with how the standard library works.
Unfortunately, we cannot use a _templated_ operator[], it would be ugly and cumbersome. However, it makes sense as a function. @Kerndog73 ?
If all of us agree on this, we have to face the most difficult thing: find the right name for that. :smile:

[Kerndog73]

@skypjack Finding it in the standard library is all you need to convince me!

[skypjack]

@Kerndog73 that's good, but operator[] isn't a viable solution, so let's propose some valid names!! :joy:

[ErikBehar]

I really think getOrAssign and for accommodate maybe think of renaming to replaceOrAssign, will make things clearer. Maybe take a poll and see what a random programmer thinks accommodate does. I bet most people will get it wrong.

[danielytics]

Many languages have a concept of map.get(key, default_value). I could imagine a version where the default_value is not just returned, but also inserted if it doesn’t exist. It sounds like this is what’s being asked for here. Whether or not its useful, I don’t know.

@skypjack getOrCreate or something to that effect maybe? (getcreate, createget, I can’t think of a single-word name)

I do agree that accommodate is not a particularly meaningful name. I had to look it up a thousand times before finally memorizing it.

[OvermindDL1]

The default_value in @danielytics suggestion could even be a lambda to construct and/or mutate-in-place the component lazily.

[skypjack]

Well, could they be just insert_or_assign instead of accommodate (even though this looks a lot like a breaking change) and something like get_or_assign or a non-const overload of get that also accepts a second parameter? Or even assure, actually.

[OvermindDL1]

I'd definitely prefer insert_or_assign, accommodate is really weird to use as a native english speaker for that...

[skypjack]

@OvermindDL1 Can I ask you why it's weird as a native speaker? Just out of curiosity. I guess it's something due to its meaning.

[OvermindDL1]

It's a word I would never think of using for that purpose or think of looking up, in addition it seems like what it would actually do is to resize something, not 'insert or assign'...

[danielytics]

I agree. Without looking it up, I would never be able to guess what it does. It sounds maybe like some sort of data formatting or casting (accommodate some data to fit some shape or something), I’d certainly not guess that it is basically an upsert operation. The dictionary definition does suit the current meaning, but it’s not a word that would immediately spring to mind for it, at least for me.

Having said that, if you don’t want to break backwards compatibility by changing it, I won’t complain.

[ErikBehar]

I guess depends from what point of view your looking at, ( insert ) sounds to me like you are looking from the point of view of the data structure, like inserting into a vector. ( assign ) sounds to me like you are looking from the ecs perspective, ie you are "attaching" a component to an entity.

In my opinion you should stick to the ecs perspective. That's why getOrAssign / replaceOrAssign make more sense to me.

[skypjack]

@ErikBehar good point. :+1:
@danielytics v3 will be already a breaking release, so who cares? :smile:

[danielytics]

@skypjack good point. Well, in that case, may as well make it clearer! I also agree with Erik’s point, sticking with the ECS perspective when naming of course makes a lot of sense.

[skypjack]

I think I'll work on this when I finished with #152 - did you find any other name that is difficult to remember/guess? Just curious.

[skypjack]

I'd rename also get_if in try_get. *_if is usually reserved to functions that accept predicates in the standard library and it could be misleading.

[skypjack]

I renamed get_if to try_get and accommodate to assign_or_replace.
Unfortunately, introducing an overload of get that also accepts an instance to assign if the entity doesn't own the component requires that the array of pools is mutable.
This is something already available in #152, so it doesn't make sense to do it twice. It means that this feature must wait until #152 is fully merged on master.

[skypjack]

No, wait, I'm quite stupid. There won't be a const overload for this function. It wouldn't make sense.
No need to wait for #152 to proceed on this actually. It was just me trying to do something wrong.

[skypjack]

Upstream.

I'll close this issue tomorrow in case of no negative feedback.
I'm also available to consider renaming other functions (if we agree they have weird names).

[ErikBehar]

I think reset is not super clear. Could be safe_remove or try_remove

[OvermindDL1]

Are you talking about the someEntity.reset<SomeComponent>() call? I agree that it does not seem to fit the standards usages for reset calls. I do like the word reset for it, but it is 'wrong' to fit the standards usages...

[skypjack]

@ErikBehar @OvermindDL1
Standard usage for reset is to (let me say) _replace_ content (think of std::unique_ptr). Probably clear fits better in this case, even though it's closer to the point of view of the container than to that of the ECS.
To be honest, safe_remove and try_remove are a bit misleading from my point of view.

[skypjack]

Well, ok, breaking changes are still possible until v3 is released.
We can safely close this issue being it done. :+1: