Skip to content

Commit

Permalink
Fix a bunch of constness issues (fixes #95).
Browse files Browse the repository at this point in the history 
- Use SFINAE to prevent non-const access to component<C>().
- Correctly de-const component types before accessing
  Component<C>::family(). Avoids accidentally assigning new family IDs.
- ComponentHandle should handle const propagation correctly now.
- ComponentHandle.manager_ should now be `const EntityManager` where
  appropriate.
  • Loading branch information
@alecthomas
alecthomas committed Apr 24, 2015
1 parent 26f02d3 commit 29d8c37
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 61 deletions.
124 changes: 66 additions & 58 deletions entityx/Entity.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <string>
#include <utility>
#include <vector>
#include <type_traits>

#include "entityx/help/Pool.h"
#include "entityx/config.h"
Expand All @@ -40,7 +41,7 @@ typedef std::uint64_t uint64_t;
class EntityManager;


template <typename C>
template <typename C, typename EM = EntityManager>
class ComponentHandle;

/** A convenience handle around an Entity::Id.
Expand Down Expand Up @@ -136,17 +137,17 @@ class Entity {
template <typename C>
void remove();

template <typename C>
template <typename C, typename = typename std::enable_if<!std::is_const<C>::value>::type>
ComponentHandle<C> component();

template <typename C>
const ComponentHandle<const C> component() const;
template <typename C, typename = typename std::enable_if<std::is_const<C>::value>::type>
const ComponentHandle<C, const EntityManager> component() const;

template <typename ... Components>
std::tuple<ComponentHandle<Components>...> components();

template <typename ... Components>
std::tuple<ComponentHandle<const Components>...> components() const;
std::tuple<ComponentHandle<const Components, const EntityManager>...> components() const;

template <typename C>
bool has_component() const;
Expand Down Expand Up @@ -176,7 +177,7 @@ class Entity {
* - If a component is removed from its host entity.
* - If its host entity is destroyed.
*/
template <typename C>
template <typename C, typename EM>
class ComponentHandle {
public:
typedef C ComponentType;
Expand Down Expand Up @@ -208,12 +209,10 @@ class ComponentHandle {
private:
friend class EntityManager;

ComponentHandle(EntityManager *manager, Entity::Id id) :
ComponentHandle(EM *manager, Entity::Id id) :
manager_(manager), id_(id) {}
ComponentHandle(const EntityManager *manager, Entity::Id id) :
manager_(const_cast<EntityManager*>(manager)), id_(id) {}

EntityManager *manager_;
EM *manager_;
Entity::Id id_;
};

Expand Down Expand Up @@ -268,7 +267,10 @@ template <typename Derived>
struct Component : public BaseComponent {
public:
typedef ComponentHandle<Derived> Handle;
typedef ComponentHandle<const Derived, const EntityManager> ConstHandle;

private:
friend class EntityManager;
/// Used internally for registration.
static Family family();
};
Expand Down Expand Up @@ -416,7 +418,7 @@ class EntityManager : entityx::help::NonCopyable {
private:
friend class EntityManager;

BaseView(EntityManager *manager) : manager_(manager) { mask_.set(); }
explicit BaseView(EntityManager *manager) : manager_(manager) { mask_.set(); }
BaseView(EntityManager *manager, ComponentMask mask) :
manager_(manager), mask_(mask) {}

Expand All @@ -431,7 +433,7 @@ class EntityManager : entityx::help::NonCopyable {
class UnpackingView {
public:
struct Unpacker {
Unpacker(ComponentHandle<Components> & ... handles) :
explicit Unpacker(ComponentHandle<Components> & ... handles) :
handles(std::tuple<ComponentHandle<Components> & ...>(handles...)) {}

void unpack(entityx::Entity &entity) const {
Expand Down Expand Up @@ -502,7 +504,7 @@ class EntityManager : entityx::help::NonCopyable {
/**
* Return true if the given entity ID is still valid.
*/
bool valid(Entity::Id id) {
bool valid(Entity::Id id) const {
return id.index() < entity_version_.size() && entity_version_[id.index()] == id.version();
}

Expand Down Expand Up @@ -572,7 +574,7 @@ class EntityManager : entityx::help::NonCopyable {
template <typename C, typename ... Args>
ComponentHandle<C> assign(Entity::Id id, Args && ... args) {
assert_valid(id);
const BaseComponent::Family family = Component<C>::family();
const BaseComponent::Family family = component_family<C>();
assert(!entity_component_mask_[id.index()].test(family));

// Placement new into the component pool.
Expand All @@ -596,7 +598,7 @@ class EntityManager : entityx::help::NonCopyable {
template <typename C>
void remove(Entity::Id id) {
assert_valid(id);
const BaseComponent::Family family = Component<C>::family();
const BaseComponent::Family family = component_family<C>();
const uint32_t index = id.index();

// Find the pool for this component family.
Expand All @@ -617,7 +619,7 @@ class EntityManager : entityx::help::NonCopyable {
template <typename C>
bool has_component(Entity::Id id) const {
assert_valid(id);
size_t family = Component<C>::family();
size_t family = component_family<C>();
// We don't bother checking the component mask, as we return a nullptr anyway.
if (family >= component_pools_.size())
return false;
Expand All @@ -632,10 +634,10 @@ class EntityManager : entityx::help::NonCopyable {
*
* @returns Pointer to an instance of C, or nullptr if the Entity::Id does not have that Component.
*/
template <typename C>
template <typename C, typename = typename std::enable_if<!std::is_const<C>::value>::type>
ComponentHandle<C> component(Entity::Id id) {
assert_valid(id);
size_t family = Component<C>::family();
size_t family = component_family<C>();
// We don't bother checking the component mask, as we return a nullptr anyway.
if (family >= component_pools_.size())
return ComponentHandle<C>();
Expand All @@ -650,17 +652,17 @@ class EntityManager : entityx::help::NonCopyable {
*
* @returns Component instance, or nullptr if the Entity::Id does not have that Component.
*/
template <typename C>
const ComponentHandle<const C> component(Entity::Id id) const {
template <typename C, typename = typename std::enable_if<std::is_const<C>::value>::type>
const ComponentHandle<C, const EntityManager> component(Entity::Id id) const {
assert_valid(id);
size_t family = Component<C>::family();
size_t family = component_family<C>();
// We don't bother checking the component mask, as we return a nullptr anyway.
if (family >= component_pools_.size())
return ComponentHandle<const C>();
return ComponentHandle<C, const EntityManager>();
BasePool *pool = component_pools_[family];
if (!pool || !entity_component_mask_[id.index()][family])
return ComponentHandle<const C>();
return ComponentHandle<const C>(this, id);
return ComponentHandle<C, const EntityManager>();
return ComponentHandle<C, const EntityManager>(this, id);
}

template <typename ... Components>
Expand All @@ -669,7 +671,7 @@ class EntityManager : entityx::help::NonCopyable {
}

template <typename ... Components>
std::tuple<ComponentHandle<const Components>...> components(Entity::Id id) const {
std::tuple<ComponentHandle<const Components, const EntityManager>...> components(Entity::Id id) const {
return std::make_tuple(component<const Components>(id)...);
}

Expand Down Expand Up @@ -751,11 +753,18 @@ class EntityManager : entityx::help::NonCopyable {
*/
void reset();

// Retrieve the component family for a type.
template <typename C>
static BaseComponent::Family component_family() {
return Component<typename std::remove_const<C>::type>::family();
}

private:
friend class Entity;
template <typename C>
template <typename C, typename EM>
friend class ComponentHandle;


inline void assert_valid(Entity::Id id) const {
assert(id.index() < entity_component_mask_.size() && "Entity::Id ID outside entity vector range");
assert(entity_version_[id.index()] == id.version() && "Attempt to access Entity via a stale Entity::Id");
Expand All @@ -764,15 +773,15 @@ class EntityManager : entityx::help::NonCopyable {
template <typename C>
C *get_component_ptr(Entity::Id id) {
assert(valid(id));
BasePool *pool = component_pools_[Component<C>::family()];
BasePool *pool = component_pools_[component_family<C>()];
assert(pool);
return static_cast<C*>(pool->get(id.index()));
}

template <typename C>
const C *get_component_ptr(Entity::Id id) const {
assert_valid(id);
BasePool *pool = component_pools_[Component<C>::family()];
BasePool *pool = component_pools_[component_family<C>()];
assert(pool);
return static_cast<const C*>(pool->get(id.index()));
}
Expand All @@ -785,7 +794,7 @@ class EntityManager : entityx::help::NonCopyable {
template <typename C>
ComponentMask component_mask() {
ComponentMask mask;
mask.set(Component<C>::family());
mask.set(component_family<C>());
return mask;
}

Expand Down Expand Up @@ -815,7 +824,7 @@ class EntityManager : entityx::help::NonCopyable {

template <typename C>
Pool<C> *accomodate_component() {
BaseComponent::Family family = Component<C>::family();
BaseComponent::Family family = component_family<C>();
if (component_pools_.size() <= family) {
component_pools_.resize(family + 1, nullptr);
}
Expand Down Expand Up @@ -869,8 +878,7 @@ ComponentHandle<C> Entity::replace(Args && ... args) {
auto handle = component<C>();
if (handle) {
*(handle.get()) = C(std::forward<Args>(args) ...);
}
else {
} else {
handle = manager_->assign<C>(id_, std::forward<Args>(args) ...);
}
return handle;
Expand All @@ -882,16 +890,16 @@ void Entity::remove() {
manager_->remove<C>(id_);
}

template <typename C>
template <typename C, typename>
ComponentHandle<C> Entity::component() {
assert(valid());
return manager_->component<C>(id_);
}

template <typename C>
const ComponentHandle<const C> Entity::component() const {
template <typename C, typename>
const ComponentHandle<C, const EntityManager> Entity::component() const {
assert(valid());
return manager_->component<const C>(id_);
return const_cast<const EntityManager*>(manager_)->component<const C>(id_);
}

template <typename ... Components>
Expand All @@ -901,9 +909,9 @@ std::tuple<ComponentHandle<Components>...> Entity::components() {
}

template <typename ... Components>
std::tuple<ComponentHandle<const Components>...> Entity::components() const {
std::tuple<ComponentHandle<const Components, const EntityManager>...> Entity::components() const {
assert(valid());
return manager_->components<const Components...>(id_);
return const_cast<const EntityManager*>(manager_)->components<const Components...>(id_);
}


Expand Down Expand Up @@ -935,44 +943,44 @@ inline std::ostream &operator << (std::ostream &out, const Entity &entity) {
}


template <typename C>
inline ComponentHandle<C>::operator bool() const {
template <typename C, typename EM>
inline ComponentHandle<C, EM>::operator bool() const {
return valid();
}

template <typename C>
inline bool ComponentHandle<C>::valid() const {
return manager_ && manager_->valid(id_) && manager_->has_component<C>(id_);
template <typename C, typename EM>
inline bool ComponentHandle<C, EM>::valid() const {
return manager_ && manager_->valid(id_) && manager_->template has_component<C>(id_);
}

template <typename C>
inline C *ComponentHandle<C>::operator -> () {
template <typename C, typename EM>
inline C *ComponentHandle<C, EM>::operator -> () {
assert(valid());
return manager_->get_component_ptr<C>(id_);
return manager_->template get_component_ptr<C>(id_);
}

template <typename C>
inline const C *ComponentHandle<C>::operator -> () const {
template <typename C, typename EM>
inline const C *ComponentHandle<C, EM>::operator -> () const {
assert(valid());
return manager_->get_component_ptr<C>(id_);
return manager_->template get_component_ptr<C>(id_);
}

template <typename C>
inline C *ComponentHandle<C>::get() {
template <typename C, typename EM>
inline C *ComponentHandle<C, EM>::get() {
assert(valid());
return manager_->get_component_ptr<C>(id_);
return manager_->template get_component_ptr<C>(id_);
}

template <typename C>
inline const C *ComponentHandle<C>::get() const {
template <typename C, typename EM>
inline const C *ComponentHandle<C, EM>::get() const {
assert(valid());
return manager_->get_component_ptr<C>(id_);
return manager_->template get_component_ptr<C>(id_);
}

template <typename C>
inline void ComponentHandle<C>::remove() {
template <typename C, typename EM>
inline void ComponentHandle<C, EM>::remove() {
assert(valid());
manager_->remove<C>(id_);
manager_->template remove<C>(id_);
}


Expand Down
18 changes: 15 additions & 3 deletions entityx/Entity_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ TEST_CASE_METHOD(EntityManagerFixture, "TestUnpack") {
// }

TEST_CASE_METHOD(EntityManagerFixture, "TestComponentIdsDiffer") {
REQUIRE(Component<Position>::family() != Component<Direction>::family());
REQUIRE(EntityManager::component_family<Position>() != EntityManager::component_family<Direction>());
}

TEST_CASE_METHOD(EntityManagerFixture, "TestEntityCreatedEvent") {
Expand Down Expand Up @@ -542,7 +542,7 @@ TEST_CASE("TestComponentDestructorCalledWhenManagerDestroyed") {
};

struct Test : Component<Test> {
Test(bool &yes) : freed(yes) {}
explicit Test(bool &yes) : freed(yes) {}

Freed freed;
};
Expand All @@ -565,7 +565,7 @@ TEST_CASE("TestComponentDestructorCalledWhenEntityDestroyed") {
};

struct Test : Component<Test> {
Test(bool &yes) : freed(yes) {}
explicit Test(bool &yes) : freed(yes) {}

Freed freed;
};
Expand All @@ -592,3 +592,15 @@ TEST_CASE_METHOD(EntityManagerFixture, "TestComponentsRemovedFromReusedEntities"
REQUIRE(!b.has_component<Position>());
b.assign<Position>(3, 4);
}

TEST_CASE_METHOD(EntityManagerFixture, "TestConstComponentsNotInstantiatedTwice") {
Entity a = em.create();
a.assign<Position>(1, 2);

const Entity b = a;

REQUIRE(a.component<Position>().valid());
REQUIRE(b.component<const Position>().valid());
REQUIRE(b.component<const Position>()->x == 1);
REQUIRE(b.component<const Position>()->y == 2);
}

0 comments on commit 29d8c37

Please sign in to comment.