Added user-defined context variable guard

[BrodyHiggerson]

Fitting with the "don't pay for what you don't use" concept, I wanted to have an optional way of injecting some kind of lock/guard around context variables in the registry.

I let the user define how the lock is created since, for me, I use a fiber mutex rather than std::mutex, so I need to be able to define it myself. I'm using the lock in the 'leaf nodes' of the function calls since I can't support recursive mutexes and the end result is the same.

Addresses #799

(Let me know if you want a PR against another branch; maybe main?)

[skypjack]

Unfortunately, this PR is no longer valid (if we consider branch wip as it ought to be, that is about to be merged with master).
Briefly, I've introduced a registry_context object here to address #575 (that makes perfectly sense to me). All functions from the registry have been moved to this object. These are the only functions available in the registry now.

I've no problem merging or reproducing the intent of this PR on a branch, adapting it and then porting it to master. There is no need for you to intervene. However, I wanted to ask you what your plans are to avoid conflicts. 🙂

[skypjack]

Actually, another option could be to turn the context type in a template parameter.
This way users can inject their own guarded maps or whatever or even suppress it. 🤔

[BrodyHiggerson]

@skypjack I actually had to comment out the

#ifndef ENTT_REGISTRY_CONTEXT_GUARD
	#define ENTT_REGISTRY_CONTEXT_GUARD()
#endif

section as include ordering was messing with it - some files/libraries would get to config via various utilities and I was struggling to always keep my own header/define before it. I think there's probably a better solution on the cards, but I'd definitely like some way to inject a custom lock either way. A template param could be nice - means the user can typedef entt::registry and use it in their code without worrying about include order shenanigans. I would just be worried about for example having that type replace the entire map and machinery and force the user to re-implement it - I want exactly what the context vars do, just with a lil' interception ;)

[TerensTare]

A little late to the party, but I think the best way to solve this is by having a mixin on top of the context. Here's the best I could get so far.

The idea is that you create a "locked context" on each thread and use it instead of the regular context. This context would be lock the given mutex on each call to emplace/etc. In code it would look something like this:

entt::registry registry;

my::mutex ctx_mutex;
entt::locked_context ctx{registry.ctx(), ctx_mutex};

// use ctx just like you would before

[skypjack]

Dang. Completely forgot about this one! 😲
I think another option is to make the context type configurable, as in:

template<typename Entity, typename Allocator>
class basic_registry;

Vs:

template<typename Entity, typename Context, typename Allocator>
class basic_registry;

The registry doesn't use it after all, therefore it doesn't really care about the API or whatever.
Roughly speaking, the context could be an std::shared_ptr<void> internally and it probably should.

[TerensTare]

Makes sense to me and looks better than the mixin solution. I'm assuming if this happens, the default context type would be the one we have now, right?

[skypjack]

Yeah, sure thing. We can also make it a trait actually 🤔 not sure it's worth it though, but I don't like much to dirty the class declaration.

[TerensTare]

If this becomes a trait, I think the trait should be passed to the registry as a template parameter. This way you can pick a custom context for every registry, which might be useful for "blueprint registries" (ie. having only blueprint entities). The code would then look something like the following:

namespace entt {
    struct default_registry_traits {
        using entity_type = entity;

        template <typename RegistryAlloc>
        using context_type = default_context<RegistryAlloc>;
    };

    template <
        typename Traits, // it's a trait now
        typename Allocator = std::allocator<typename Traits::entity_type>>
    class basic_registry;
}

struct my_traits : entt::default_registry_traits {
    using entity_type = uint64_t;
};

The reason why the Allocator is not specified in the traits are as following:

1. To break the least amount of code possible. If it were in the traits, the registry would now have one template parameter as opposed to two, which would break code already using a custom allocator.
2. The allocator depends on the entity type, and having to specify it on every specialization can be bug prone. Consider the previous example again:

struct my_traits : entt::default_registry_traits {
    using entity_type = uint64_t;
    // oops, using the allocator for entt::entity...
};

[skypjack]

The other option is to use something on the line of custom data in meta. That is:

auto &elem = registry.ctx<my_context_type>();
elem.emplace<my_type>(42, 'c');

Where the current type is also the default type. This means that registry.ctx() is also backward compatible.
Internally, we only need to allocate the context object and store it with a type id, to debug-assert on use if needed.
This way, one can decide to use a different context type and the whole thing is imho even cleaner than the traits machinery.
Does it make sense? Granted, accessing the context suffers from an extra indirection here, but is it a problem anyway? I don't see the registry context as something used on hot paths in general, it wouldn't make much sense imho.

[TerensTare]

I keep saying it again and again, but I like this idea better than the traits. The only thing worth to discuss is the lifetime of the context. Given that now the context type will be dynamic internally, there could be an option to change it at runtime, similar to custom data in meta (eg. the context is initially an int variable but then you set it to be a string). I would argue against that, as it feels more natural to reason about code when the type doesn't change and there is no benefit to resetting the context to a different type/value (I think).

If one truly needs the feature, it can easily be emulated by making the context represent several states within the type:

struct context final
{
    // represent states in one type, instead of having separate types of contexts
    bool is_state_1() const;
    bool is_state_2() const;
    // ...

private:
    // internal data
};

However, this means the only way to set the context value (not modify, just set) is when constructing the registry. To me, the simplest way to do this is to have a basic_any<0u> parameter to the registry's constructor that defaults to the "default-context" value; I don't see a reason to have a templated constructor instead of basic_any.

[skypjack]

there could be an option to change it at runtime

Yes and no actually. Imagine the following:

template<typename Type = DefaultType>
auto & ctx() {
    ENTT_ASSERT(context_pair.second == nullptr || context_pair.first == type_id<Type>(), "Mismatch");

    if(context_pair.second == nullptr) {
        context_pair.first = type_id<Type>();
        context_pair.second = std::make_shared<Type>();
    }

    return *static_pointer_cast<Type>(context_pair.second);
}

Where context_pair is something like std::pair<type_info, std::shared_ptr<void>>.
All off the top of my head, I don't think this snippet even compiles, but it should give you an idea of ​​what I mean.
In this case, you can't change the context type at runtime basically, you get an assert and you know it. Once you decide, it's that for the life of the registry. Just... with a lazy type.

[TerensTare]

I have a slightly different idea. I'm guessing that shared_ptr is used for the type-erased deleter. However, the shared_ptr has some extra overhead in memory (allocating the control block, which stores more than just the object). Also, there will only ever be one copy of the context, so there is no need for shared ownership. Then, I would argue that the pair stores the type_info even if not used (ie. when assertions are disabled). type_id "caches" each type_info into static storage, so I think having a reference instead is probably lighter than holding the type_info itself.

Having said that, I think it's best to drop the pair for a unique_ptr<void, Deleter>. The deleter doubles up as a way to retrieve the type_info of the context when needed, or rather a reference to it. In total, this would need just two pointers to work: the pointer to the context, and the pointer to the deleter, and only one member, the unique_ptr. The changes are minimal and the code inside ctx is cleaner (subjective).

Here's the code.

[skypjack]

The deleter doubles up as a way to retrieve the type_info of the context when needed

This is REALLY smart. I'd probably use a class with an extra member rather than the same operator() function for everything, but still. A really smart idea.

[skypjack]

Ah! You know why this could be tricky maybe? Because of uses-allocator construction and because we should use the registry allocator to instantiate the context. 🤔

[TerensTare]

This is REALLY smart.

Thanks, good to hear that. 🙂

I'd probably use a class with an extra member rather than the same operator() function for everything.

That's fine by me. I only did it like this because it's shorter.

Also probably worth mentioning, but I just remembered that delete works with nullptr as well (does nothing), so it's perfectly fine if we remove the pointer check on the vtable. I tested with the same code as earlier, and there was no problem.

EDIT

We should use the registry allocator to instantiate the context. 🤔

We can always store a pointer to the allocator in the deleter and use it to destroy/deallocate the context. The type of allocator is already known so there is no much (if any) type-erasure needed for this.

[skypjack]

Yeah, I'm trying to figure out if we can make allocate_unique a little more flexible. It doesn't worth it though probably.

[TerensTare]

It's probably better to leave allocate_unique as is. It shouldn't be too hard to sneak an allocator in the code. This should be good enough to add allocator support (potentially free for empty allocators), at least for what we discussed so far.