From a68fd253a1cb60a32fb26283cab23d1d5da57054 Mon Sep 17 00:00:00 2001
From: Vincent <magicdroidx@gmail.com>
Date: Wed, 22 Jan 2025 22:08:03 +0000
Subject: [PATCH] refactor: correct implementation of `OwnerStorageEntity` and
 `OwnerPtr`

---
 .../gamerefs_entity/owner_storage_entity.h    | 19 ++++------
 .../stack_result_storage_entity.cpp           | 13 +++++--
 .../owner_storage_shareptr.h                  | 12 +++---
 src/bedrock/gamerefs/owner_ptr.h              | 37 +++++++++++++++++--
 src/bedrock/gamerefs/stack_ref_result.h       |  2 +-
 src/endstone/core/level/level.cpp             | 10 ++---
 6 files changed, 62 insertions(+), 31 deletions(-)

diff --git a/src/bedrock/entity/gamerefs_entity/owner_storage_entity.h b/src/bedrock/entity/gamerefs_entity/owner_storage_entity.h
index fd2592b88..7a1a983fe 100644
--- a/src/bedrock/entity/gamerefs_entity/owner_storage_entity.h
+++ b/src/bedrock/entity/gamerefs_entity/owner_storage_entity.h
@@ -20,26 +20,23 @@
 #include "bedrock/entity/gamerefs_entity/entity_context.h"
 
 class OwnerStorageEntity {
-public:
-    [[nodiscard]] EntityContext &getStackRef() const
-    {
-        if (!context_.has_value()) {
-            throw std::bad_optional_access();
-        }
-        return const_cast<EntityContext &>(context_.value());
-    }
+protected:
+    OwnerStorageEntity() = default;
+    OwnerStorageEntity(const OwnerStorageEntity &) = delete;
+    OwnerStorageEntity &operator=(const OwnerStorageEntity &) = delete;
 
-    [[nodiscard]] bool hasValue() const
+    [[nodiscard]] bool _hasValue() const
     {
         return context_.has_value();
     }
 
-    [[nodiscard]] operator bool() const noexcept
+    [[nodiscard]] EntityContext &_getStackRef() const
     {
-        return hasValue();
+        return const_cast<EntityContext &>(context_.value());
     }
 
 private:
+    friend class StackResultStorageEntity;
     std::optional<EntityContext> context_;
 };
 BEDROCK_STATIC_ASSERT_SIZE(OwnerStorageEntity, 32, 32);
diff --git a/src/bedrock/entity/gamerefs_entity/stack_result_storage_entity.cpp b/src/bedrock/entity/gamerefs_entity/stack_result_storage_entity.cpp
index c39a546a7..4086d8b45 100644
--- a/src/bedrock/entity/gamerefs_entity/stack_result_storage_entity.cpp
+++ b/src/bedrock/entity/gamerefs_entity/stack_result_storage_entity.cpp
@@ -27,14 +27,19 @@ StackResultStorageEntity::StackResultStorageEntity(WeakStorageEntity const &weak
     }
 }
 
-StackResultStorageEntity::StackResultStorageEntity(EntityContext const &context)
+StackResultStorageEntity::StackResultStorageEntity(EntityContext const &entity)
 {
-    if (context.isValid()) {
-        context_.emplace(context);
+    if (entity.isValid()) {
+        context_.emplace(entity);
     }
 }
 
-StackResultStorageEntity::StackResultStorageEntity(OwnerStorageEntity const &ref) : context_(ref.getStackRef()) {}
+StackResultStorageEntity::StackResultStorageEntity(OwnerStorageEntity const &owner_storage)
+{
+    if (owner_storage._hasValue()) {
+        context_.emplace(owner_storage.context_.value());
+    }
+}
 
 bool StackResultStorageEntity::_hasValue() const
 {
diff --git a/src/bedrock/gamerefs/gamerefs_shareptr/owner_storage_shareptr.h b/src/bedrock/gamerefs/gamerefs_shareptr/owner_storage_shareptr.h
index 8332f095c..89ed6415d 100644
--- a/src/bedrock/gamerefs/gamerefs_shareptr/owner_storage_shareptr.h
+++ b/src/bedrock/gamerefs/gamerefs_shareptr/owner_storage_shareptr.h
@@ -18,15 +18,17 @@
 
 template <typename T>
 class OwnerStorageSharePtr {
-public:
-    T &operator*() const
+protected:
+    OwnerStorageSharePtr() = default;
+
+    [[nodiscard]] bool _hasValue() const
     {
-        return *value_;
+        return value_ != nullptr;
     }
 
-    T *operator->() const noexcept
+    T &_getStackRef() const
     {
-        return value_.get();
+        return *value_;
     }
 
 private:
diff --git a/src/bedrock/gamerefs/owner_ptr.h b/src/bedrock/gamerefs/owner_ptr.h
index a90c5f19f..1e0a96c95 100644
--- a/src/bedrock/gamerefs/owner_ptr.h
+++ b/src/bedrock/gamerefs/owner_ptr.h
@@ -16,7 +16,38 @@
 
 #include "bedrock/gamerefs/gamerefs_shareptr/gamerefs_shareptr.h"
 
-template <typename T>
-class OwnerPtr : public GameRefs<T>::OwnerStorage {
-    using GameRefs<T>::OwnerStorage::OwnerStorage;
+template <typename Type>
+class OwnerPtr : public GameRefs<Type>::OwnerStorage {
+public:
+    using StackRef = typename GameRefs<Type>::StackRef;
+    using GameRefs<Type>::OwnerStorage::OwnerStorage;
+
+    explicit operator bool() const
+    {
+        return hasValue();
+    }
+
+    [[nodiscard]] bool hasValue() const
+    {
+        return GameRefs<Type>::OwnerStorage::_hasValue();
+    }
+
+    StackRef &operator*() const
+    {
+        return value();
+    }
+
+    StackRef *operator->() const
+    {
+        return &value();
+    }
+
+    StackRef &value() const
+    {
+        return GameRefs<Type>::OwnerStorage::_getStackRef();
+    }
+
+    WeakRef<Type> getWeakRef() const;
+    bool operator==(nullptr_t) const;
+    bool operator!=(nullptr_t) const;
 };
diff --git a/src/bedrock/gamerefs/stack_ref_result.h b/src/bedrock/gamerefs/stack_ref_result.h
index 4e2847ded..ef9b5bfd4 100644
--- a/src/bedrock/gamerefs/stack_ref_result.h
+++ b/src/bedrock/gamerefs/stack_ref_result.h
@@ -27,7 +27,7 @@ class StackRefResult : public GameRefs<Type>::StackResultStorage {
         return hasValue();
     }
 
-    bool hasValue() const  // NOLINT(*-use-nodiscard)
+    [[nodiscard]] bool hasValue() const
     {
         return GameRefs<Type>::StackResultStorage::_hasValue();
     }
diff --git a/src/endstone/core/level/level.cpp b/src/endstone/core/level/level.cpp
index 77dae3bd9..49e1cbb2a 100644
--- a/src/endstone/core/level/level.cpp
+++ b/src/endstone/core/level/level.cpp
@@ -49,21 +49,17 @@ std::string EndstoneLevel::getName() const
 std::vector<Actor *> EndstoneLevel::getActors() const
 {
     std::vector<Actor *> result;
-    for (const auto &e : level_.getEntities()) {
-        if (!e.hasValue()) {
+    for (const auto &entity : level_.getEntities()) {
+        if (!entity.hasValue()) {
             continue;
         }
-
-        // TODO(check): is this the correct usage of OwnerPtr<EntityContext> ?
-        const auto *actor = ::Actor::tryGetFromEntity(e.getStackRef(), false);
+        const auto *actor = ::Actor::tryGetFromEntity(*entity, false);
         if (!actor) {
             continue;
         }
-
         if (&actor->getLevel() != &level_) {
             continue;
         }
-
         result.push_back(&actor->getEndstoneActor());
     }
     return result;