Skip to content

Commit

Permalink
ECS fix for layout calculations (#115)
Browse files Browse the repository at this point in the history 
* Remove unnecessary indirection

* Remove unncessary intermediary function

* Fix ECS bug where the wrong component was used for layout calculations

Caused fairly bad bugs when components with different widths/alignments are intermixed

* Enhance ECS test for finding layout differences
  • Loading branch information
@seanmiddleditch
seanmiddleditch committed Jul 19, 2019
1 parent aeb7262 commit a52f971
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 44 deletions.
2 changes: 1 addition & 1 deletion source/library/ecs/private/archetype.h
View file
Expand Up @@ -18,7 +18,7 @@ struct up::World::Archetype {

int32 indexOfLayout(ComponentId component) const noexcept {
for (int32 i = 0; i != static_cast<int32>(layout.size()); ++i) {
if (layout[i].meta->id == component) {
if (layout[i].component == component) {
return i;
}
}
Expand Down
52 changes: 29 additions & 23 deletions source/library/ecs/private/world.cpp
View file
Expand Up @@ -29,7 +29,7 @@ up::World::World() = default;

up::World::~World() = default;

void up::World::_calculateLayout(uint32 archetypeIndex, view<ComponentMeta const*> components) {
void up::World::_populateArchetype(uint32 archetypeIndex, view<ComponentMeta const*> components) {
Archetype& archetype = *_archetypes[archetypeIndex];

archetype.layout.resize(components.size());
Expand All @@ -47,38 +47,42 @@ void up::World::_calculateLayout(uint32 archetypeIndex, view<ComponentMeta const
}
UP_ASSERT(size <= sizeof(Chunk::data));

if (size != 0) {
_calculateLayout(archetype, size);
}

// Required for partial match algorithm
sort(archetype.layout, {}, &Layout::component);
}

void up::World::_calculateLayout(Archetype& archetype, size_t size) {
// calculate layout hash
archetype.layoutHash = hashComponents(span{archetype.layout}, &Layout::component);

if (size != 0) {
// assign pointer offers by alignment
up::sort(archetype.layout, {}, [](const auto& layout) noexcept { return layout.meta->alignment; });

// FIXME: figure out why we need this size + 1, the arithmetic surprises me
archetype.perChunk = static_cast<uint32>(sizeof(Chunk::Payload) / (size + 1));
// assign pointer offers by alignment
up::sort(archetype.layout, {}, [](const auto& layout) noexcept { return layout.meta->alignment; });

// we'll always include the EntityId is the layout, so include it as offset
size_t offset = sizeof(EntityId) * archetype.perChunk;
// FIXME: figure out why we need this size + 1, the arithmetic surprises me
archetype.perChunk = static_cast<uint32>(sizeof(Chunk::Payload) / (size + 1));

for (size_t i = 0; i != components.size(); ++i) {
ComponentMeta const& meta = *components[i];
// we'll always include the EntityId is the layout, so include it as offset
size_t offset = sizeof(EntityId) * archetype.perChunk;

// align as required (requires alignment to be a power of 2)
UP_ASSERT((meta.alignment & (meta.alignment - 1)) == 0);
offset = align(offset, meta.alignment);
UP_ASSERT(offset + archetype.perChunk * meta.size + meta.size <= sizeof(Chunk::Payload));
for (size_t i = 0; i != archetype.layout.size(); ++i) {
ComponentMeta const& meta = *archetype.layout[i].meta;

archetype.layout[i].offset = static_cast<uint32>(offset);
archetype.layout[i].width = meta.size;
// align as required (requires alignment to be a power of 2)
UP_ASSERT((meta.alignment & (meta.alignment - 1)) == 0);
offset = align(offset, meta.alignment);
UP_ASSERT(offset + archetype.perChunk * meta.size + meta.size <= sizeof(Chunk::Payload));

offset += meta.size * archetype.perChunk;
}
archetype.layout[i].offset = static_cast<uint32>(offset);
archetype.layout[i].width = meta.size;

UP_ASSERT(offset <= sizeof(Chunk::data));
offset += meta.size * archetype.perChunk;
}

// Required for partial match algorithm
sort(archetype.layout, {}, &Layout::component);
UP_ASSERT(offset <= sizeof(Chunk::data));
}

void up::World::deleteEntity(EntityId entity) noexcept {
Expand Down Expand Up @@ -285,7 +289,7 @@ auto up::World::_findArchetypeIndex(view<ComponentMeta const*> components) noexc
auto arch = new_box<Archetype>();
_archetypes.push_back(std::move(arch));
uint32 archetypeIndex = static_cast<uint32>(_archetypes.size() - 1);
_calculateLayout(archetypeIndex, components);
_populateArchetype(archetypeIndex, components);

UP_ASSERT(_archetypes[archetypeIndex]->layoutHash == hash);

Expand Down Expand Up @@ -335,6 +339,8 @@ auto up::World::_createEntityRaw(view<ComponentMeta const*> components, view<voi
ComponentMeta const& meta = *components[index];

int32 layoutIndex = archetype.indexOfLayout(meta.id);
UP_ASSERT(archetype.layout[layoutIndex].meta == &meta);
UP_ASSERT(archetype.layout[layoutIndex].width == meta.size);
UP_ASSERT(layoutIndex != -1);

void* rawPointer = chunk.pointer(archetype.layout[layoutIndex], subIndex);
Expand Down
12 changes: 3 additions & 9 deletions source/library/ecs/public/potato/ecs/query.h
View file
Expand Up @@ -36,9 +36,8 @@ namespace up {
void select(World& world, Delegate callback) const;

private:
void _invoke(size_t count, EntityId const* entities, view<void*> pointers, Delegate callback) const;
template <size_t... Indices>
void _invokeHelper(std::index_sequence<Indices...>, size_t, EntityId const*, view<void*>, Delegate callback) const;
void _invoke(std::index_sequence<Indices...>, size_t, EntityId const*, view<void*>, Delegate& callback) const;

ComponentId _components[sizeof...(Components)];
ComponentId _sortedComponents[sizeof...(Components)];
Expand All @@ -62,19 +61,14 @@ namespace up {

template <typename... Components>
template <size_t... Indices>
void Query<Components...>::_invokeHelper(std::index_sequence<Indices...>, size_t count, EntityId const* entities, view<void*> arrays, Delegate callback) const {
void Query<Components...>::_invoke(std::index_sequence<Indices...>, size_t count, EntityId const* entities, view<void*> arrays, Delegate& callback) const {
callback(count, entities, static_cast<Components*>(arrays[Indices])...);
}

template <typename... Components>
void Query<Components...>::_invoke(size_t count, EntityId const* entities, view<void*> pointers, Delegate callback) const {
_invokeHelper(std::make_index_sequence<sizeof...(Components)>(), count, entities, pointers, callback);
}

template <typename... Components>
void Query<Components...>::select(World &world, Delegate callback) const {
world.selectRaw(_sortedComponents, _components, [&, this](size_t count, EntityId const* entities, view<void*> arrays) {
this->_invoke(count, entities, arrays, callback);
this->_invoke(std::make_index_sequence<sizeof...(Components)>(), count, entities, arrays, callback);
});
}
} // namespace up
3 changes: 2 additions & 1 deletion source/library/ecs/public/potato/ecs/world.h
View file
Expand Up @@ -79,7 +79,8 @@ namespace up {
UP_ECS_API void _addComponentRaw(EntityId entityId, ComponentMeta const* componentMeta, void const* componentData) noexcept;

void _deleteLocation(Location const& location) noexcept;
void _calculateLayout(uint32 archetypeIndex, view<ComponentMeta const*> components);
void _populateArchetype(uint32 archetypeIndex, view<ComponentMeta const*> components);
static void _calculateLayout(Archetype& archetype, size_t size);
bool _matchArchetype(uint32 archetypeIndex, view<ComponentId> sortedComponents) const noexcept;
void _selectChunksRaw(uint32 archetypeIndex, view<ComponentId> components, delegate_ref<RawSelectSignature> callback) const;
void _recycleEntityId(EntityId entity) noexcept;
Expand Down
20 changes: 10 additions & 10 deletions source/library/ecs/tests/test_world.cpp
View file
Expand Up @@ -6,11 +6,11 @@ struct Test1 {
char a;
};
struct Second {
char a;
float b;
char a;
};
struct Another {
float a;
double a;
float b;
};
struct Counter {
Expand All @@ -29,9 +29,9 @@ DOCTEST_TEST_SUITE("[potato][ecs] World") {
DOCTEST_TEST_CASE("Archetype selects") {
World world;

world.createEntity(Test1{'f'}, Second{'g', 7.f});
world.createEntity(Another{1.f, 2.f}, Second{'g', 9.f});
world.createEntity(Second{'h', -2.f}, Another{2.f, 1.f});
world.createEntity(Test1{'f'}, Second{7.f, 'g'});
world.createEntity(Another{1.f, 2.f}, Second{9.f, 'g'});
world.createEntity(Second{-2.f, 'h'}, Another{2.f, 1.f});
world.createEntity(Test1{'j'}, Another{3.f, 4.f});

// Exactly two of the entities should be in the same archetype
Expand Down Expand Up @@ -64,9 +64,9 @@ DOCTEST_TEST_SUITE("[potato][ecs] World") {
DOCTEST_TEST_CASE("Direct component access") {
World world;

world.createEntity(Test1{'f'}, Second{'g', 7.f});
world.createEntity(Another{1.f, 2.f}, Second{'g', 9.f});
auto id = world.createEntity(Second{'h', -2.f}, Another{2.f, 1.f});
world.createEntity(Test1{'f'}, Second{7.f, 'g'});
world.createEntity(Another{1.f, 2.f}, Second{9.f, 'g'});
auto id = world.createEntity(Second{-2.f, 'h'}, Another{2.f, 1.f});

Second* test = world.getComponentSlow<Second>(id);
DOCTEST_CHECK(test != nullptr);
Expand All @@ -77,8 +77,8 @@ DOCTEST_TEST_SUITE("[potato][ecs] World") {
DOCTEST_TEST_CASE("EntityId management") {
World world;

EntityId first = world.createEntity(Test1{'f'}, Second{'g', 7.f});
EntityId second = world.createEntity(Test1{'h'}, Second{'i', -1.f});
EntityId first = world.createEntity(Test1{'f'}, Second{7.f, 'g'});
EntityId second = world.createEntity(Test1{'h'}, Second{-1.f, 'i'});

Query<Test1> query;
query.select(world, ([&](size_t count, EntityId const* ids, Test1*) {
Expand Down

0 comments on commit a52f971

Please sign in to comment.