Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mixins working with custom protected registry #1225

Open
wants to merge 1 commit into
base: wip
Choose a base branch
from

Conversation

actondev
Copy link

Working but.. not-confident-about-the-whole-thing addressing #1222
The issue is I don't quite like this reinterpret_cast there

What I don't quite understand, is that both the if statements get hit

  • when constructing the custom registry, the first if statement gets hit, this is on the basic_registry constructor when calling rebind. How come any_cast detects the custom registry?
  • when creating storages from assure (let's say from .emplace<Component> from the custom registry, assuming it was exposed), the newly created pool binding hits the second if and the reinterpret_cast is needed.

template<typename Entity>
struct basic_custom_registry: entt::basic_registry<Entity> {};
class basic_custom_registry: protected entt::basic_registry<Entity> {
using registry_type = entt::basic_registry<Entity>;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a clang-format file in EnTT. Please, use it also for these changes. 🙏🙂

@@ -130,7 +130,12 @@ class basic_sigh_mixin final: public Type {
}

void bind_any(any value) noexcept final {
owner = internal::any_to_owner<registry_type>(value);
if(auto *registry = any_cast<owner_type>(&value)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather change the code of any_to_owner than duplicate the logic in two places.
Also, I agree that reinterpret_cast is a no-go. Probably a static_cast is more than enough and slightly safer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants