entt icon indicating copy to clipboard operation
entt copied to clipboard

Registry `ctx` redesign: no equivalent for `ctx_or_set()`

Open cschreib opened this issue 3 years ago • 10 comments

Hello! I just recently tried to upgrade from EnTT 3.9.0 to 3.10.1, and noticed this version is using a re-design of the registry ctx(). I was able to find the substitutions for simple cases:

  • r.ctx<T>() -> r.ctx().at<T>()
  • r.set<T>() -> r.ctx().emplace<T>()

But I did not find a substitute for r.ctx_or_set<T>(), which comes in handy. Do I have to hand-roll it like so?

auto& x = r.ctx().contains<T>() ? r.ctx().at<T>() : r.ctx().emplace<T>();

If so, would it be possible to add this feature back into the new design, as at_or_emplace()?

(also, I'm sure that ship has sailed now, but at doesn't read very well and differs from the terminology used in the registry for regular components; get would have been easier for muscle memory!)

cschreib avatar Jul 27 '22 08:07 cschreib

Hello! 🙂 The emplace and emplace_hint functions invoke try_emplace on a dense_map internally. The latter (doc quote):

Inserts in-place if the key does not exist, does nothing if the key exists

I think this is what you're looking for to replace ctx_or_set, right? As for the following instead:

(also, I'm sure that ship has sailed now, but at doesn't read very well and differs from the terminology used in the registry for regular components; get would have been easier for muscle memory!)

You're not alone 🙂 I've used at because of dense_map::at to which we forward the call. However, this isn't the first time that someone complains about this name. So, yeah, expect to see it deprecated in the next version in favor of get. 👍

skypjack avatar Jul 27 '22 08:07 skypjack

Actually, please, leave this issue open so that I won't forget about at and get. Thanks! 😉

skypjack avatar Jul 27 '22 08:07 skypjack

Thanks for your reply!

I think this is what you're looking for to replace ctx_or_set, right?

Yes, it is actually. I didn't look at the dense_map documentation, expecting that just looking at context would be enough (after all, the dense_map is an implementation detail right?), and that the function semantic was the same as that of registry. But that is not the case: in registry, calling emplace() with a component that already exists is an error, if you're unsure, you need to use either of emplace_or_replace() or get_or_emplace() depending on the semantic you need (and I think that's good design, as it forces you to be explicit about what you want).

So at the moment there's a mismatch between the two classes, which is confusing given that they are so close to each other:

registry context
emplace() missing
emplace_or_replace() missing
replace() missing
erase() erase()
get() at()
get_or_emplace() emplace()
try_get() find()
all_of() contains()

It would be neat if the names were aligned. Right now it feels like the semantic of context is inherited from dense_map, which seems like a leaky abstraction. As a user, I don't really care that dense_map is used under the hood, I care more about having a good and consistent interface, if that makes sense.

You're not alone slightly_smiling_face I've used at because of dense_map::at to which we forward the call. However, this isn't the first time that someone complains about this name. So, yeah, expect to see it deprecated in the next version in favor of get. +1

Oh OK, nice :)

cschreib avatar Jul 28 '22 12:07 cschreib

Actually my first post is incorrect, r.ctx().emplace<T>(...) is not a correct replacement for r.set<T>(...) as emplace() does nothing if the value is already set, but set() always replaced it :thinking: That's tricky.

cschreib avatar Jul 28 '22 12:07 cschreib

Well, technically speaking, the context is a key-value map while the registry is way more complex than this. That is why the API of the context is designed around that of a map now. However, I agree that at<T> doesn't really fit. 🙂

skypjack avatar Jul 28 '22 14:07 skypjack

Ok, setting aside semantic for a second then :) One of the use case I have for the context is to store a boolean "editor mode" flag that, when set to true, enables operations that are normally not allowed.

I used to use it like so:

r.set<game::editor_mode>(true);
// Do stuff ...
r.set<game::editor_mode>(false);

With the current API (assuming get() makes it in), i'd have to do

r.ctx().get<game::editor_mode>() = true;
// Do stuff ...
r.ctx().get<game::editor_mode>() = false;

... which isn't very nice (getX() = Y always seems like an antipattern), and also relies on the flag being emplace()d somewhere else before. It seems to me the old API was more user friendly in this case.

I can always write my own wrapper functions around this, but I thought I'd share the feedback!

cschreib avatar Jul 28 '22 14:07 cschreib

So, a sort of insert_or_assign<T>(args...) aside the emplace function?

skypjack avatar Jul 28 '22 14:07 skypjack

That would do, yes! Although as with "at" vs "get", and the naming debate above, I would question the use of "insert", which is very "container-centric". To me, the context isn't a container into which I want to insert and find things, it's an object in which I can set and query global state (that state is stored in a container, true, but that isn't relevant to me).

cschreib avatar Jul 28 '22 15:07 cschreib

insert_or_assign_hint is actually very heavy 😅 but also emplace_or_replace_hint is bad, so 🤷‍♂️ I'll stick with the container similarity I guess. Any suggestions?

skypjack avatar Aug 01 '22 13:08 skypjack

Not sure "hint" is the right word. Usually (at least in the STL) a hint is something that you're providing to an algorithm to help it find the correct answer faster, but the algorithm can ignore the hint and should return the same answer. That doesn't seem to fit here.

What about "as"? emplace_or_replace_as("centre_of_universe"_hs, position{0, 0}) doesn't read too bad.

cschreib avatar Aug 01 '22 16:08 cschreib