Repository: mlange-42/arche
Pull request #216 (2023-04-13 22:08:11)

Fix: Always panic on dead entity
All world methods with an entity as argument panic on a dead/recycled entity; causes 0.5ns slower `World.Get(Entity)`


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 807363f..389f016 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,10 @@
 
 * Adds method `World.ComponentType(ID)` to get the `reflect.Type` for component IDs (#215)
 
+### Bugfixes
+
+* All world methods with an entity as argument panic on a dead/recycled entity; causes 0.5ns slower `World.Get(Entity)` (#216)
+
 ## [[v0.6.3]](https://github.com/mlange-42/arche/compare/v0.6.2...v0.6.3)
 
 ### Documentation
diff --git a/README.md b/README.md
index bc5dae5..8c12b04 100644
--- a/README.md
+++ b/README.md
@@ -172,7 +172,7 @@ In the illustration, the first archetype holds all components for all entities w
 The exact composition of each archetype is encoded in a bitmask for fast comparison.
 Thus, queries can easily identify their relevant archetypes, and then simply iterate entities linearly, which is very fast. Components can be accessed through the query in a very efficient way (&approx;1ns).
 
-For getting components by entity ID, e.g. for hierarchies, the world contains a list that is indexed by the entity ID. For each entity, it references it's current archetype and the index in the archetype. This way, getting components for entity IDs (i.e. random access) is fast, although not as fast as in queries (≈1.5ns vs. 1ns).
+For getting components by entity ID, e.g. for hierarchies, the world contains a list that is indexed by the entity ID. For each entity, it references it's current archetype and the index in the archetype. This way, getting components for entity IDs (i.e. random access) is fast, although not as fast as in queries (≈1.8ns vs. 1ns).
 
 Obviously, archetypes are an optimization for iteration speed.
 But they also come with a downside. Adding or removing components to/from an entity requires moving all the components of the entity to another archetype.
diff --git a/benchmark/profile/world/main.go b/benchmark/profile/world/main.go
index 41aa00a..b87f7f9 100644
--- a/benchmark/profile/world/main.go
+++ b/benchmark/profile/world/main.go
@@ -44,9 +44,10 @@ func run(rounds, iters, entityCount int) {
 		posID := ecs.ComponentID[position](&world)
 		rotID := ecs.ComponentID[rotation](&world)
 
-		entities := make([]ecs.Entity, entityCount)
-		for i := 0; i < entityCount; i++ {
-			entities[i] = world.NewEntity(posID, rotID)
+		entities := make([]ecs.Entity, 0, entityCount)
+		query := world.Batch().NewEntitiesQuery(entityCount, posID, rotID)
+		for query.Next() {
+			entities = append(entities, query.Entity())
 		}
 
 		for j := 0; j < iters; j++ {
diff --git a/ecs/pool.go b/ecs/pool.go
index f67b7f6..dfc44ea 100644
--- a/ecs/pool.go
+++ b/ecs/pool.go
@@ -65,7 +65,7 @@ func (p *entityPool) Reset() {
 	p.available = 0
 }
 
-// Alive return whether an entity is still alive, based on the entity's generations.
+// Alive returns whether an entity is still alive, based on the entity's generations.
 func (p *entityPool) Alive(e Entity) bool {
 	return e.id != 0 && e.gen == p.entities[e.id].gen
 }
diff --git a/ecs/world.go b/ecs/world.go
index f1dfd95..509a8f0 100644
--- a/ecs/world.go
+++ b/ecs/world.go
@@ -244,11 +244,17 @@ func (w *World) newEntitiesWithQuery(count int, comps ...Component) Query {
 
 // RemoveEntity removes and recycles an [Entity].
 //
+// Panics when called for a removed (and potentially recycled) entity.
+//
 // Panics when called on a locked world or for an already removed entity.
 // Do not use during [Query] iteration!
 func (w *World) RemoveEntity(entity Entity) {
 	w.checkLocked()
 
+	if !w.entityPool.Alive(entity) {
+		panic("can't remove a dead entity")
+	}
+
 	index := &w.entities[entity.id]
 	oldArch := index.arch
 
@@ -314,31 +320,38 @@ func (w *World) Alive(entity Entity) bool {
 	return w.entityPool.Alive(entity)
 }
 
-// Get returns a pointer th the given component of an [Entity].
-//
+// Get returns a pointer to the given component of an [Entity].
 // Returns nil if the entity has no such component.
-// Panics when called for an already removed entity.
+//
+// Panics when called for a removed (and potentially recycled) entity.
 //
 // See also [github.com/mlange-42/arche/generic.Map.Get] for a generic variant.
 func (w *World) Get(entity Entity, comp ID) unsafe.Pointer {
+	if !w.entityPool.Alive(entity) {
+		panic("can't get component of a dead entity")
+	}
 	index := &w.entities[entity.id]
 	return index.arch.Get(index.index, comp)
 }
 
 // Has returns whether an [Entity] has a given component.
 //
-// Panics when called for an already removed entity.
+// Panics when called for a removed (and potentially recycled) entity.
 //
 // See also [github.com/mlange-42/arche/generic.Map.Has] for a generic variant.
 func (w *World) Has(entity Entity, comp ID) bool {
+	if !w.entityPool.Alive(entity) {
+		panic("can't check for component of a dead entity")
+	}
 	return w.entities[entity.id].arch.HasComponent(comp)
 }
 
 // Add adds components to an [Entity].
 //
-// Panics when called with component that can't be added because they are already present.
-// Panics when called on a locked world or for an already removed entity.
-// Do not use during [Query] iteration!
+// Panics:
+//   - when called for a removed (and potentially recycled) entity.
+//   - when called with components that can't be added because they are already present.
+//   - when called on a locked world. Do not use during [Query] iteration!
 //
 // Note that calling a method with varargs in Go causes a slice allocation.
 // For maximum performance, pre-allocate a slice of component IDs and pass it using ellipsis:
@@ -358,9 +371,10 @@ func (w *World) Add(entity Entity, comps ...ID) {
 // The components in the Comp field of [Component] must be pointers.
 // The passed pointers are no valid references to the assigned memory!
 //
-// Panics when called with components that can't be added because they are already present.
-// Panics when called on a locked world or for an already removed entity.
-// Do not use during [Query] iteration!
+// Panics:
+//   - when called for a removed (and potentially recycled) entity.
+//   - when called with components that can't be added because they are already present.
+//   - when called on a locked world. Do not use during [Query] iteration!
 //
 // See also the generic variants under [github.com/mlange-42/arche/generic.Map1], etc.
 func (w *World) Assign(entity Entity, comps ...Component) {
@@ -390,10 +404,10 @@ func (w *World) Assign(entity Entity, comps ...Component) {
 // Returns a pointer to the assigned memory.
 // The passed in pointer is not a valid reference to that memory!
 //
-// Panics when called on a locked world or for an already removed entity.
-// Do not use during [Query] iteration!
-//
-// Panics if the entity does not have a component of that type.
+// Panics:
+//   - when called for a removed (and potentially recycled) entity.
+//   - if the entity does not have a component of that type.
+//   - when called on a locked world. Do not use during [Query] iteration!
 //
 // See also [github.com/mlange-42/arche/generic.Map.Set] for a generic variant.
 func (w *World) Set(entity Entity, id ID, comp interface{}) unsafe.Pointer {
@@ -402,9 +416,10 @@ func (w *World) Set(entity Entity, id ID, comp interface{}) unsafe.Pointer {
 
 // Remove removes components from an entity.
 //
-// Panics when called with components that can't be removed because they are not present.
-// Panics when called on a locked world or for an already removed entity.
-// Do not use during [Query] iteration!
+// Panics:
+//   - when called for a removed (and potentially recycled) entity.
+//   - when called with components that can't be removed because they are not present.
+//   - when called on a locked world. Do not use during [Query] iteration!
 //
 // See also the generic variants under [github.com/mlange-42/arche/generic.Map1], etc.
 func (w *World) Remove(entity Entity, comps ...ID) {
@@ -413,15 +428,19 @@ func (w *World) Remove(entity Entity, comps ...ID) {
 
 // Exchange adds and removes components in one pass.
 //
-// Panics when called with components that can't be added or removed because
-// they are already present/not present, respectively.
-// Panics when called on a locked world or for an already removed entity.
-// Do not use during [Query] iteration!
+// Panics:
+//   - when called for a removed (and potentially recycled) entity.
+//   - when called with components that can't be added or removed because they are already present/not present, respectively.
+//   - when called on a locked world. Do not use during [Query] iteration!
 //
 // See also the generic variants under [github.com/mlange-42/arche/generic.Exchange].
 func (w *World) Exchange(entity Entity, add []ID, rem []ID) {
 	w.checkLocked()
 
+	if !w.entityPool.Alive(entity) {
+		panic("can't exchange components on a dead entity")
+	}
+
 	if len(add) == 0 && len(rem) == 0 {
 		return
 	}
@@ -542,6 +561,9 @@ func (w *World) IsLocked() bool {
 //
 // Can be used for fast checks of the entity composition, e.g. using a [Filter].
 func (w *World) Mask(entity Entity) Mask {
+	if !w.entityPool.Alive(entity) {
+		panic("can't exchange components on a dead entity")
+	}
 	return w.entities[entity.id].arch.Mask
 }
 


Test patch:
diff --git a/ecs/world_test.go b/ecs/world_test.go
index a81d485..2d255a6 100644
--- a/ecs/world_test.go
+++ b/ecs/world_test.go
@@ -237,6 +237,10 @@ func TestWorldExchange(t *testing.T) {
 
 	assert.Panics(t, func() { w.Exchange(e1, []ID{velID}, []ID{}) })
 	assert.Panics(t, func() { w.Exchange(e1, []ID{}, []ID{posID}) })
+
+	w.RemoveEntity(e0)
+	_ = w.NewEntity()
+	assert.Panics(t, func() { w.Exchange(e0, []ID{posID}, []ID{}) })
 }
 
 func TestWorldAssignSet(t *testing.T) {
@@ -286,7 +290,12 @@ func TestWorldAssignSet(t *testing.T) {
 	*pos = Position{8, 9}
 	pos = (*Position)(w.Get(e2, posID))
 	assert.Equal(t, 8, pos.X)
+
+	w.RemoveEntity(e0)
+	_ = w.NewEntity()
+	assert.Panics(t, func() { w.Assign(e0, Component{posID, &Position{2, 3}}) })
 }
+
 func TestWorldGetComponents(t *testing.T) {
 	w := NewWorld()
 
@@ -316,6 +325,12 @@ func TestWorldGetComponents(t *testing.T) {
 	assert.Equal(t, &Position{100, 101}, pos1)
 
 	w.RemoveEntity(e0)
+	assert.Panics(t, func() { w.Get(e0, posID) })
+	assert.Panics(t, func() { w.Mask(e0) })
+
+	_ = w.NewEntity(posID)
+	assert.Panics(t, func() { w.Get(e0, posID) })
+	assert.Panics(t, func() { w.Mask(e0) })
 
 	pos1 = (*Position)(w.Get(e1, posID))
 	assert.Equal(t, &Position{100, 101}, pos1)
@@ -964,6 +979,30 @@ func printTypeSizeName[T any](name string) {
 	fmt.Printf("%18s: %5d B\n", name, tp.Size())
 }
 
+func BenchmarkEntityAlive_1000(b *testing.B) {
+	b.StopTimer()
+
+	world := NewWorld(NewConfig().WithCapacityIncrement(1024))
+	posID := ComponentID[Position](&world)
+
+	entities := make([]Entity, 0, 1000)
+	q := world.newEntitiesQuery(1000, posID)
+	for q.Next() {
+		entities = append(entities, q.Entity())
+	}
+
+	b.StartTimer()
+
+	var alive bool
+	for i := 0; i < b.N; i++ {
+		for _, e := range entities {
+			alive = world.Alive(e)
+		}
+	}
+
+	_ = alive
+}
+
 func BenchmarkGetResource(b *testing.B) {
 	b.StopTimer()