Entt: [Feature request] Express the mutability of components in the template parameters of a view

[Kerndog73]

I thought I'd put this discussion in an issue so that it doesn't get lost in Gitter.

The problem with a const view not modifying components is that a copy of the const view can be made and the components can be modifyed with the copy. The fact that a const view cannot modify components really doesn't make much sense. A const std::shared_ptr can still be used to modify the stored object. A const std::span can still be used to modify the stored object.

The mutability (or const-ness as I like to call it) should be part of the template parameters of the view. This is similar to how std::span works. A std::span<const int> cannot be used to modify a sequence of integers but a const std::span<int> can.

Consider this:

// The view object is const because we are not going to "look at something else"
const auto view = reg.view<const A, B>();
// the type of the view is view<entity_type, const A, B>
for (const auto e : view) {
  //       B &        =    const A &
  view.get<B>(e).data = view.get<A>(e).data;
}

Components being looked at by a const view can be modified. The template parameters tell the view which components can be modified and which cannot. Knowing which components a system reads and writes is valuable information, especially when parallelism is involved. Declaring the mutability in the template parameters not only documents read/write components but also enforces that read-only components cannot be written. Notice from the example that view.get<A>(e) returns a const A & because A is declared as const in the template parameter.

Determining whether a component is declared const in the template parameters requires iterating the type list. This is what I came up with:

template <typename Comp, typename... Comps>
struct find_comp;

template <typename Comp, typename... Tail>
struct find_comp<Comp, Comp, Tail...> {
  using type = Comp;
};

template <typename Comp, typename Head, typename... Tail>
struct find_comp<Comp, Head, Tail...> {
  using type = std::conditional_t<
    std::is_same_v<std::add_const_t<Comp>, Head>,
    Head,
    typename find_comp<Comp, Tail...>::type
  >;
};

template <typename Comp>
struct find_comp<Comp> {
  using type = void;
};

static_assert(std::is_same_v<typename find_comp<int, int>::type, int>);
static_assert(std::is_same_v<typename find_comp<int, const int>::type, const int>);
static_assert(std::is_same_v<typename find_comp<int, char, short, int, long>::type, int>);
static_assert(std::is_same_v<typename find_comp<int, char, short, const int, long>::type, const int>);

After fiddling around with the multi-component view for a while I managed to get this test to succeed.

auto view = reg.view<const int, long>();
for (const auto e : view) {
  static_assert(std::is_same_v<const int &, decltype(view.get<int>(e))>);
  static_assert(std::is_same_v<long &, decltype(view.get<long>(e))>);
}

My changes were rather sloppy which is why this is an issue and not a pull request. The modification to the view mostly consisted of putting std::remove_const_t in pool_type and component_iterator_type, then declaring this:

template <typename Comp>
using ret_type = typename find_comp<Comp, Component...>::type;

and making get return ret_type<Comp> &. I also modified the registry by doing this:

template<typename... Component>
View<Entity, Component...> view() {
    return View<Entity, Component...>{(
        assure<std::remove_const_t<Component>>(), 
        pool<std::remove_const_t<Component>>()
    )...};
}

Whether or not a const registry should return views to const components is a separate discussion but I think it makes sense.

[skypjack]

This is similar to how std::span works. A std::span cannot be used to modify a sequence of integers but a const std::span can. This feature is another example of EnTT following the conventions of the standard library.

Despite the fact that I'd like to adhere to this model because it makes sense, are you sure EnTT follows the conventions of the standard library?
I cannot test it right now, but I'm not that sure template machinery under the hood has all the decay calls correctly set so as to follow this convention. Of course, this would only be a reason to update it in this sense.

[Kerndog73]

I had to put std::remove_const_t around the place to get it to work. By "follows the conventions of the standard library" I was referring to the lowercase names for classes. std::decay_t is a better choice but I forgot it existed.

[skypjack]

I had to put std::remove_const_t around the place to get it to work. By "follows the conventions of the standard library" I was referring to the lowercase names for classes. std::decay_t is a better choice but I forgot it existed

Yeah, something to work on so as to follow the conventions then. :+1:

Declaring the mutability in the template parameters not only documents read/write components but also enforces that read-only components cannot be written.

Never thought at it this way, but I must admit that I really like your point of view. It will take a while to review the whole codebase in this sense, but probably it's worth it. Let's see.

[skypjack]

Actually the standard library is quite a can of worms because of its inconsistency on this conventions. :joy:

[skypjack]

At a first glance (keep in mind that I cannot work on it right now, so it's just a fast review of what we have) it doesn't make sense to create pools of const components nor to ask to the registry to _do things_ (for some definitions of _doing things_) with const components BUT it makes sense to create views that mix both const and non-const components. It's also something probably easier to do and to maintain as a convention.
Was this what you were looking for? It would allow us to create const views from the registry somehow, even though I've yet to realize how to proceed.

[Kerndog73]

I believe that views should have mutation characteristics of std::span (const if template parameter is const) and the registry should have mutation characteristics of std::vector (const if object is const).

Whether or not sparse sets behave like std::span or std::vector is up to you. Views can always do the appropriate casting.

[skypjack]

I believe that views should have mutation characteristics of std::span (const if template parameter is const) and the registry should have mutation characteristics of std::vector (const if object is const).

We have a deal. This is what makes sense to me too.
Let's see if we can implement something like this. :+1:

[skypjack]

What's wrong with it is this. If I push a const registry into a function, I don't want it to modify components. However, the function can still create a view for A instead of const A and update components.
We should static assert constness of requested components in the const overload of the view member function of the registry in order to forbid it.

[Kerndog73]

We could static_assert or we could std::add_const_t to the component types. When you request a non-const iterator from a const std::vector (via begin()) you are given a const_iterator.

Do we want to follow the standard library or follow our own needs? I'm leaning towards the standard library approach.

[skypjack]

Yeah, well, I had the _nazi-mode_ turned on probably. :joy:
Btw you got the point, const overload of registry::view must return a view only for const access. Otherwise it defeats completely the purpose of being const. :+1:

[skypjack]

Have a look at branch experimental, in particular to class raw_view and member function registry::raw_view.
Is that what you were looking for? Take your time to review it and let me know your thoughts. Thanks.

What doesn't fit very well so far is if you look at persistent_views. They offer two member functions, initialize and sort, that doesn't make much sense to define as const.
On the other side, ie for raw_view, all the member functions of a view are now const, so it's strange as well to have only two member functions that _diverge_ somehow from the policy.

What about?

[Kerndog73]

I wasn't expecting you to start working on this feature so quickly!

There are a few places where you std::decay_t when I don't think you need to. The main reason for decaying the type is so that component_family::type gives us the right ID. So it makes sense to pass the undecayed type through all the layers until the type actually matters like when we need to get a component ID or construct a component.

I don't think registry::raw needs to decay. What if we have a non-const registry and we ask for a const component array? It would make sense to do reg.raw<const Comp>() but the component type is decayed so we don't get a const component array. The const overload of raw should just std::add_const_t to the return type. I think my feedback on this function can be applied to a lot of places.

I don't want the addition of this feature to leave const_cast and std::decay_t scattered across the library.

I don't really understand why registry::assign decays it's return type. To me, assign<const Comp> means "assign to Comp and give me a const reference to it". In that case, assign doesn't need to change.

Once again, registry::get decays when I don't think it should. I don't think this function needs to change.

Both replace and accommodate construct a decayed component which is good because you can't move-from a const object. However, the return type is decayed. Similar to assign, I don't think the return type needs to be decayed.

The view functions std::remove_reference_t which I think is interesting. What does a user expect when they ask for a view onto component references? I think we should just let the view notice that they're dealing with a reference and try to do something invalid with it. If there was previously a compile-time error when asking for a view onto component references then I think that behavior should continue.

[skypjack]

I wasn't expecting you to start working on this feature so quickly!

To be honest, this is something that could also help me while designing software. Sometimes I had to find workarounds to the fact that I couldn't create a view from a const registry.

The main reason for decaying the type is so that component_family::type gives us the right ID

In fact, no. component_family::type already decays internally. :-)

I don't think registry::raw needs to decay. What if we have a non-const registry and we ask for a const component array? It would make sense to do reg.raw<const Comp>() but the component type is decayed so we don't get a const component array. The const overload of raw should just std::add_const_t to the return type. I think my feedback on this function can be applied to a lot of places.

Uhm, ok, it makes sense. Btw we still need two functions here, const and non-const. Unfortunately, the const overload cannot only add const, because it wouldn't work as expected in some cases (ie if Component is a reference). So, in that case, we still have to remove the reference somehow and then add a const. Details anyway.

I don't want the addition of this feature to leave const_cast and std::decay_t scattered across the library.

Unfortunately, there aren't much alternatives. The registry and the views will contain their own set of const_casts at the end of the changes. I don't see any other solution other than const_cast the pools.
I'm open to suggestions if you come up with a good idea.

I don't really understand why registry::assign decays it's return type. To me, assign means "assign to Comp and give me a const reference to it". In that case, assign doesn't need to change.

Once again, registry::get decays when I don't think it should. I don't think this function needs to change.

Both replace and accommodate construct a decayed component which is good because you can't move-from a const object. However, the return type is decayed. Similar to assign, I don't think the return type needs to be decayed.

It makes sense. I can't see much uses for these, but it makes sense. Approved.
However, assign<const T>(...) could be misleading, because one could expect the registry to assign an instance of const T to an entity, that doesn't make much sense.

The view functions std::remove_reference_t which I think is interesting. What does a user expect when they ask for a view onto component references? I think we should just let the view notice that they're dealing with a reference and try to do something invalid with it. If there was previously a compile-time error when asking for a view onto component references then I think that behavior should continue.

There will be a compile-time error, of course. Btw we can expect a decay to be to the call site. :+1:
That being side, ignore view and persistent_view, they have not been updated yet. runtime_view was ready for that without changes, raw_view is the only one that I modified to its (more or less) final version so far.

[skypjack]

Update: with some mutable put in the right places, the number of const_casts required by this enhancement decreased to 0 so far. :+1:

[skypjack]

My two cents.

const overload of registry.view should not allow for non-const types as template parameters. It's misleading.
Consider this:

• Non-const overload: registry.view<int, const char>() returns entt::view<int, const char>.
• const overload: registry.view<int, const char>() returns silently entt::view<const int, const char>.

I wouldn't expect such a behavior as a client. Therefore, later I'd be tempted to do view.get<int>(id); because of obvious reasons and everything just explodes with a wall of errors as usual with templates.
I think it's by far better to force using const types as template parameter when you create a view through the const overload, so that errors are caught earlier and the whole thing is less ambiguous.

[Kerndog73]

I understand your point of view but usually you know whether the registry you're dealing with is const. You can expect the behavior. "I know the registry is const so I expect this behavior". If you knew that the registry was const then you would never write reg.view<int, const char>(), you would write reg.view<int, char>() and expect the view function to std::add_const_t.

Const is great. Having a const registry automatically give you const views is really convenient. You write const once and receive const objects everywhere.

[skypjack]

I'll think twice about this later. It's easy to change anyway.
Let's see if someone else has a different point of view and decides to share it with us. :+1:

You write const once and receive const objects everywhere.

Actually this seems a good point.

[skypjack]

~Multi component standard views are yet to be updated. All the others are theoretically already ready to use.~
All the views have been updated and everything seems to work like a charm. I've to write some tests and to update the documentation yet. Moreover, I want to review everything to see if I can optimize things or make it easier to read. In other terms, it's almost ready to go, but it will take a while before to see it on master.
In the meantime, would you help me with a review of the code and some tests your side to check that everything works as expected? It could happen that you spot some errors I missed this way.
Thanks.

[Kerndog73]

I like your use of disjunction instead the templates in the initial post. It really makes things cleaner although I'm not entirely sure what disjunction does so I'll have to read up on that. I'll have a play around with this within the next couple of hours.

[Kerndog73]

OK, so disjunction is a short-circuiting logical or that returns ::value instead of a bool. That's pretty cool.

---

I ran the test that I initially wrote for this feature and it failed 😢.

void print(entt::registry<> &reg) {
  auto view = reg.view<const int, long>();
  for (const auto e : view) {
    static_assert(std::is_same_v<const int &, decltype(view.get<int>(e))>);
    static_assert(std::is_same_v<long &, decltype(view.get<long>(e))>);
    std::cout << view.get<int>(e) << ' ' << view.get<long>(e) << '\n';
    view.get<long>(e) = 7;
  }
}

void assignIntLong(entt::registry<> &reg, const int i) {
  const auto e = reg.create();
  reg.assign<int>(e, i);
  reg.assign<long>(e, i * 2);
}

int main() {
  entt::registry<> reg;
  assignIntLong(reg, 5);
  assignIntLong(reg, 49);
  assignIntLong(reg, 72);
  assignIntLong(reg, 101);
  print(reg);
  return 0;
}

---

view.get<int>(e) fails to compile. I got this message:

./entt/entity/view.hpp:631:13: error: static_assert failed due to requirement 'std::disjunction_v<std::is_same<int, const int>, std::is_same<int, long>, std::is_same<std::remove_const_t<int>, const int>,
      std::is_same<std::remove_const_t<int>, long> >'
            static_assert(std::disjunction_v<std::is_same<Comp..., Component>..., std::is_same<std::remove_const_t<Comp>..., Component>...>);

Component is const int, long and Comp is int. The disjunction should call std::remove_const_t on both.

static_assert(std::disjunction_v<std::is_same<std::remove_const_t<Comp>..., std::remove_const_t<Component>>...>);

---

The next error is type not found in type list

./entt/entity/view.hpp:473:21: note: in instantiation of function template specialization 'std::__1::get<entt::sparse_set<unsigned int, int> *, const entt::sparse_set<unsigned int, int> *,
      entt::sparse_set<unsigned int, long> *>' requested here
        return std::get<pool_type<comp_type> *>(pools);
                    ^
./entt/entity/view.hpp:632:21: note: in instantiation of function template specialization 'entt::view<unsigned int, const int, long>::pool<int>' requested here
            return (pool<Comp>()->get(entity), ...);
                    ^

So the pools tuple is const sparse_int *, sparse_long * and we can't find sparse_int *. We're call pool<int>() and we should get a const sparse_int *. This expression pool_type<comp_type> * (line 473) should be const sparse_int *. This is the definition of comp_type (line 472)

using comp_type = std::conditional_t<std::disjunction_v<std::is_same<Comp, Component>...>, Comp, std::remove_const_t<Comp>>;

Since int is not equal to either const int or long, the typecomp_type is std::remove_const_t<int>. So now we want pool_type<int> * to yield const sparse_int *. This is the definition of pool_type (line 381)

template<typename Comp>
    using pool_type = std::conditional_t<std::is_const_v<Comp>, const sparse_set<Entity, std::remove_const_t<Comp>>, sparse_set<Entity, Comp>>;

Since int is not const, this yields sparse_int. If we go back to the error at line 473 we see this:

std::get<sparse_int *>(pools)

We want pool_type to give us a const sparse_int which means that we need to give it const int. We want comp_type to give us const int.

If the Comp template parameter given to pool() (line 470) is const or the type in Component is const, comp_type should be const. Otherwise, comp_type should be non-const.

using comp_type = std::conditional_t<
  // If the Comp template parameter is const or
  std::is_const_v<Comp> ||
  // the type in Component is const,
  std::disjunction_v<std::is_same<std::add_const_t<Comp>, Component>...>,
  // comp_type should be const.
  std::add_const_t<Comp>,
  // Otherwise, comp_type should be non-const
  Comp
>;

---

The test succeeds!

I changed the parameter to print to const entt::registry<> &reg and it still succeeds. Then I changed to print function to this:

void print(entt::registry<> &reg) {
  auto view = reg.view<const int, long>();
  for (const auto e : view) {
    static_assert(std::is_same_v<const int &, decltype(view.get<int>(e))>);
    static_assert(std::is_same_v<long &, decltype(view.get<long>(e))>);
    static_assert(std::is_same_v<const int &, decltype(view.get<const int>(e))>);
    static_assert(std::is_same_v<const long &, decltype(view.get<const long>(e))>);
    std::cout << view.get<int>(e) << ' ' << view.get<long>(e) << '\n';
  }
}

It fails at view.get<const long>(e).

---

It's another type not found in type list error. Here it is

./entt/entity/view.hpp:473:21: note: in instantiation of function template specialization 'std::__1::get<const entt::sparse_set<unsigned int, long> *, const entt::sparse_set<unsigned int, int> *,
      entt::sparse_set<unsigned int, long> *>' requested here
        return std::get<pool_type<comp_type> *>(pools);

Oh dear...

I'm afraid my replacement for comp_type may not behave as it should. Since the given Comp parameter is const long, comp_type is const long so pool_type<const long> is const sparse_long *. The pools are const sparse_int *, sparse_long *. What we really want to do is find sparse_long * and cast that to const sparse_long *. The logic I put in comp_type is in the wrong place.

comp_type should always yield one of the types in Component. If Component is const int, long then this is a table of what comp_type should be.

| Comp | comp_type |
| --- | --- |
| int | const int |
| long | long |
| const int | const int |
| const long | long |

This is the new version of comp_type that behaves as the table

using comp_type = std::conditional_t<
  std::disjunction_v<std::is_same<std::add_const_t<Comp>, Component>...>,
  std::add_const_t<Comp>,
  std::remove_const_t<Comp>
>;

I've just noticed that this is very similar to your version. I just put in std::add_const_t.

---

Now this is the only test that is failing

static_assert(std::is_same_v<const long &, decltype(view.get<const long>(e))>);

Making the pool const when the Component is const is handled by comp_type. We still need to make the pool const if Comp is const. We just need to insert that at line 473.

using ret_pool_type = std::conditional_t<
  std::is_const_v<Comp>,
  const pool_type<comp_type>,
  pool_type<comp_type>
>;
return static_cast<ret_pool_type *>(std::get<pool_type<comp_type> *>(pools));

---

Just to recap. Here is a diff of my changes

@@ -469,8 +469,17 @@

     template<typename Comp>
     auto * pool() const ENTT_NOEXCEPT {
-        using comp_type = std::conditional_t<std::disjunction_v<std::is_same<Comp, Component>...>, Comp, std::remove_const_t<Comp>>;
-        return std::get<pool_type<comp_type> *>(pools);
+        using comp_type = std::conditional_t<
+          std::disjunction_v<std::is_same<std::add_const_t<Comp>, Component>...>,
+          std::add_const_t<Comp>,
+          std::remove_const_t<Comp>
+        >;
+        using ret_pool_type = std::conditional_t<
+          std::is_const_v<Comp>,
+          const pool_type<comp_type>,
+          pool_type<comp_type>
+        >;
+        return static_cast<ret_pool_type *>(std::get<pool_type<comp_type> *>(pools));
     }

     const view_type * candidate() const ENTT_NOEXCEPT {
@@ -628,7 +637,7 @@
         assert(contains(entity));

         if constexpr(sizeof...(Comp) == 1) {
-            static_assert(std::disjunction_v<std::is_same<Comp..., Component>..., std::is_same<std::remove_const_t<Comp>..., Component>...>);
+            static_assert(std::disjunction_v<std::is_same<std::remove_const_t<Comp>..., std::remove_const_t<Component>>...>);
             return (pool<Comp>()->get(entity), ...);
         } else {
             return std::tuple<Comp &...>{get<Comp>(entity)...};

and this is the test that previously failed but now succeeds.

void print(entt::registry<> &reg) {
  auto view = reg.view<const int, long>();
  for (const auto e : view) {
    static_assert(std::is_same_v<const int &, decltype(view.get<int>(e))>);
    static_assert(std::is_same_v<long &, decltype(view.get<long>(e))>);
    static_assert(std::is_same_v<const int &, decltype(view.get<const int>(e))>);
    static_assert(std::is_same_v<const long &, decltype(view.get<const long>(e))>);
  }
}

---

I wasn't expecting to spend this long on this! I hope what I've done today is helpful.

[skypjack]

Yeah, sure. First of all, thank you very much!! Really, really appreciated.
I'll review your suggestions. What's really, really important is your test!! I've still to write them and you helped me a lot to spot errors.
I'ts midnight here, so do not trust me much. However, at a first glance, I wouldn't make the same change you did, because probably we can obtain the same effect with another changes that is smaller. I'll try all of this tomorrow, now I just need to sleep :smile:

Would you mind if I get you test, adapt it somehow and put it in the test suite?

[Kerndog73]

Do whatever you want with my test. I don't mind.

[skypjack]

Wait. The test is _wrong_ (at least, the way I thought I would have used this feature).
If you do this:

auto view = registry.view<const int, float>();

You cannot do this at the moment:

view.get<int>()

Instead you should do this:

view.get<const int>()

The problem is that the code would be misleading otherwise and I should go always to the declaration site of the view to know if a type is const or not.

Did you expect it instead?
I'm not that sure it's a good idea actually. I'll sleep on this and see if it makes sense. :+1:

If we decide to go for this, than I can confirm immediately that all your tests will fail, because this isn't something supported so far. Types must match with the ones used to construct the view. This is also why I said it was confusing the fact that the const overload implicitly adds a const for you.

[Kerndog73]

I've thought about it and I think I agree with you. It's not immediately clear what view.get<int>() returns. I'm pretty sure this is how the code behaved before I went and messed it up!

[skypjack]

To be honest, I prefer the way it is right now. When you want const, write const.
I see your point when you say - _I want to write const once and get const everywhere_. Btw it's not that clear at the call site what's going on and the resulting code is a bit odd and hard to read.

Some examples.

registry.view<S, T>();

If registry is const, you obtain a view<const S, const T>, otherwise you receive a view<S, T>. Chances are that you guess it or scroll back and search for the definition of registry when you read your own code two months later.
On the other side, being explicit would help a lot:

registry.view<const S, const T>();

The same applies to:

view.get<S>();

What I'm going to receive? A const reference to S? A reference to S? Will I be able to set a data member or not? I don't know.
At a first glance, a sane reader would say S & and try to modify it. If the view had been created using const S, the resulting error is quite indecipherable instead.

And so on. We can easily make lot of examples where this approach could be misleading.
I vote for the other way around. A little bit more verbose but by far easier to read and to work with.
My two cents.

[skypjack]

Added tests and doc. From my point of view, this is ready to merge and I'm quite satisfied with the result.
Let me know your thoughts. I'll merge in on master during the week if no errors pop out on this issue. :+1: