Fix component overriding when in disorder (#15)

* Fix component overriding when in disorder

* Add benchmarks

* Should use same data

benches/query.lua

@@ -9,13 +9,16 @@ local function TITLE(title: string)
 end
 
 local jecs = require("../mirror/init")
-local ecs = jecs.World.new()
 
+local oldMatter = require("../oldMatter")
 
+local newMatter = require("../newMatter")
 type i53 = number
 
-do TITLE (testkit.color.white_underline("query"))
+do TITLE (testkit.color.white_underline("Jecs query"))
+    local ecs = jecs.World.new()
     do TITLE "one component in common"
+
         local function view_bench(
             world: jecs.World,
             A: i53, B: i53, C: i53, D: i53, E: i53, F: i53, G: i53, H: i53
@@ -61,37 +64,37 @@ do TITLE (testkit.color.white_underline("query"))
 
             if flip() then 
                 combination ..= "B"
-                ecs:set(entity, D2, true)
+                ecs:set(entity, D2, {value = true})
             end
             if flip() then 
                 combination ..= "C"
-                ecs:set(entity, D3, true)
+                ecs:set(entity, D3, { value = true })
             end
             if flip() then 
                 combination ..= "D"
-                ecs:set(entity, D4, true)
+                ecs:set(entity, D4, { value = true})
             end
             if flip() then 
                 combination ..= "E"
-                ecs:set(entity, D5, true)
+                ecs:set(entity, D5, { value = true})
             end
             if flip() then 
                 combination ..= "F"
-                ecs:set(entity, D6, true)
+                ecs:set(entity, D6, {value = true})
             end
             if flip() then 
                 combination ..= "G"
-                ecs:set(entity, D7, true)
+                ecs:set(entity, D7, { value = true})
             end
             if flip() then 
                 combination ..= "H"
-                ecs:set(entity, D8, true)
+                ecs:set(entity, D8, {value = true})
 
             end
 
             if #combination == 7 then 
                 added += 1
-                ecs:set(entity, D1, true)
+                ecs:set(entity, D1, { value = true})
             end
 			archetypes[combination] = true
         end
@@ -101,4 +104,196 @@ do TITLE (testkit.color.white_underline("query"))
 
         view_bench(ecs, D1, D2, D3, D4, D5, D6, D7, D8)
     end
+end
+
+do TITLE(testkit.color.white_underline("OldMatter query")) 
+
+    local ecs = oldMatter.World.new()
+    local component = oldMatter.component
+
+    do TITLE "one component in common"
+        local function view_bench(
+            world: jecs.World,
+            A: i53, B: i53, C: i53, D: i53, E: i53, F: i53, G: i53, H: i53
+        )
+
+            BENCH("1 component", function()
+                for _ in world:query(A) do end
+            end)
+
+            BENCH("2 component", function()
+                for _ in world:query(A, B) do end
+            end)
+
+            BENCH("4 component", function()
+                for _ in world:query(A, B, C, D) do 
+                end
+            end)
+
+            BENCH("8 component", function()
+                for _ in world:query(A, B, C, D, E, F, G, H) do end
+            end)
+        end
+
+        local D1 = component()
+        local D2 = component()
+        local D3 = component()
+        local D4 = component()
+        local D5 = component()
+        local D6 = component()
+        local D7 = component()
+        local D8 = component()
+
+        local function flip() 
+            return math.random() >= 0.15
+        end
+
+        local added = 0
+		local archetypes = {}
+        for i = 1, 2^16-2 do 
+            local entity = ecs:spawn()
+
+            local combination = ""
+
+            if flip() then 
+                combination ..= "B"
+                ecs:insert(entity, D2({value =  true}))
+            end
+            if flip() then 
+                combination ..= "C"
+                ecs:insert(entity, D3({value =  true}))
+            end
+            if flip() then 
+                combination ..= "D"
+                ecs:insert(entity, D4({value =  true}))
+            end
+            if flip() then 
+                combination ..= "E"
+                ecs:insert(entity, D5({value =  true}))
+            end
+            if flip() then 
+                combination ..= "F"
+                ecs:insert(entity, D6({value =  true}))
+
+            end
+            if flip() then 
+                combination ..= "G"
+                ecs:insert(entity, D7({value =  true}))
+
+            end
+            if flip() then 
+                combination ..= "H"
+                ecs:insert(entity, D8({value =  true}))
+            end
+
+            if #combination == 7 then 
+                added += 1
+                ecs:insert(entity, D1({value =  true}))
+
+            end
+			archetypes[combination] = true
+        end
+
+		local a = 0
+		for _ in archetypes do a+= 1 end
+
+        view_bench(ecs, D1, D2, D3, D4, D5, D6, D7, D8)
+    end
+
+end
+
+do TITLE(testkit.color.white_underline("NewMatter query")) 
+
+    local ecs = newMatter.World.new()
+    local component = newMatter.component
+
+    do TITLE "one component in common"
+        local function view_bench(
+            world: jecs.World,
+            A: i53, B: i53, C: i53, D: i53, E: i53, F: i53, G: i53, H: i53
+        )
+
+            BENCH("1 component", function()
+                for _ in world:query(A) do end
+            end)
+
+            BENCH("2 component", function()
+                for _ in world:query(A, B) do end
+            end)
+
+            BENCH("4 component", function()
+                for _ in world:query(A, B, C, D) do 
+                end
+            end)
+
+            BENCH("8 component", function()
+                for _ in world:query(A, B, C, D, E, F, G, H) do end
+            end)
+        end
+
+        local D1 = component()
+        local D2 = component()
+        local D3 = component()
+        local D4 = component()
+        local D5 = component()
+        local D6 = component()
+        local D7 = component()
+        local D8 = component()
+
+        local function flip() 
+            return math.random() >= 0.15
+        end
+
+        local added = 0
+		local archetypes = {}
+        for i = 1, 2^16-2 do 
+            local entity = ecs:spawn()
+
+            local combination = ""
+
+            if flip() then 
+                combination ..= "B"
+                ecs:insert(entity, D2({value =  true}))
+            end
+            if flip() then 
+                combination ..= "C"
+                ecs:insert(entity, D3({value =  true}))
+            end
+            if flip() then 
+                combination ..= "D"
+                ecs:insert(entity, D4({value =  true}))
+            end
+            if flip() then 
+                combination ..= "E"
+                ecs:insert(entity, D5({value =  true}))
+            end
+            if flip() then 
+                combination ..= "F"
+                ecs:insert(entity, D6({value =  true}))
+
+            end
+            if flip() then 
+                combination ..= "G"
+                ecs:insert(entity, D7({value =  true}))
+
+            end
+            if flip() then 
+                combination ..= "H"
+                ecs:insert(entity, D8({value =  true}))
+            end
+
+            if #combination == 7 then 
+                added += 1
+                ecs:insert(entity, D1({value =  true}))
+
+            end
+			archetypes[combination] = true
+        end
+
+		local a = 0
+		for _ in archetypes do a+= 1 end
+
+        view_bench(ecs, D1, D2, D3, D4, D5, D6, D7, D8)
+    end
+
 end

lib/init.lua

@@ -51,31 +51,48 @@ local REST = HI_COMPONENT_ID + 4
 
 local function transitionArchetype(
 	entityIndex: EntityIndex,
-	destinationArchetype: Archetype,
+	to: Archetype,
 	destinationRow: i24,
-	sourceArchetype: Archetype,
+	from: Archetype,
 	sourceRow: i24
 )
-	local columns = sourceArchetype.columns
+	local columns = from.columns
-	local sourceEntities = sourceArchetype.entities
+	local sourceEntities = from.entities
-	local destinationEntities = destinationArchetype.entities
+	local destinationEntities = to.entities
-	local destinationColumns = destinationArchetype.columns
+	local destinationColumns = to.columns
+	local tr = to.records
+	local types = from.types
 
-	for componentId, column in columns do
+	for i, column in columns do
-		local targetColumn = destinationColumns[componentId]
+		-- Retrieves the new column index from the source archetype's record from each component 
+		-- We have to do this because the columns are tightly packed and indexes may not correspond to each other.
+		local targetColumn = destinationColumns[tr[types[i]]]
+
+		-- Sometimes target column may not exist, e.g. when you remove a component.
 		if targetColumn then 
 			targetColumn[destinationRow] = column[sourceRow]
 		end
-		column[sourceRow] = column[#column]
+		-- If the entity is the last row in the archetype then swapping it would be meaningless.
-		column[#column] = nil
+		local last = #column
+		if sourceRow ~= last then 
+			-- Swap rempves columns to ensure there are no holes in the archetype.
+			column[sourceRow] = column[last]
+		end
+		column[last] = nil
 	end
 
+	-- Move the entity from the source to the destination archetype.
 	destinationEntities[destinationRow] = sourceEntities[sourceRow]
 	entityIndex[sourceEntities[sourceRow]].row = destinationRow
 
+	-- Because we have swapped columns we now have to update the records
+	-- corresponding to the entities' rows that were swapped.
 	local movedAway = #sourceEntities
-	sourceEntities[sourceRow] = sourceEntities[movedAway]
+	if sourceRow ~= movedAway then 
-	entityIndex[sourceEntities[movedAway]].row = sourceRow
+		sourceEntities[sourceRow] = sourceEntities[movedAway]
+		entityIndex[sourceEntities[movedAway]].row = sourceRow
+	end
+	
 	sourceEntities[movedAway] = nil
 end
 
@@ -145,7 +162,9 @@ local function archetypeOf(world: World, types: { i24 }, prev: Archetype?): Arch
 	}
 	world.archetypeIndex[ty] = archetype
 	world.archetypes[id] = archetype
-	createArchetypeRecords(world.componentIndex, archetype, prev)
+	if #types > 0 then 
+		createArchetypeRecords(world.componentIndex, archetype, prev)
+	end
 
 	return archetype
 end
@@ -180,8 +199,6 @@ local function emit(world, eventDescription)
 	})
 end
 
-
-
 local function onNotifyAdd(world, archetype, otherArchetype, row: number, added: Ty) 
 	if #added > 0 then 
 		emit(world, {
@@ -194,13 +211,13 @@ local function onNotifyAdd(world, archetype, otherArchetype, row: number, added:
 	end
 end
 
-
 export type World = typeof(World.new())
 
 local function ensureArchetype(world: World, types, prev)
 	if #types < 1 then
 		return world.ROOT_ARCHETYPE
 	end
+	
 	local ty = hash(types)
 	local archetype = world.archetypeIndex[ty]
 	if archetype then
@@ -226,8 +243,13 @@ end
 
 local function findArchetypeWith(world: World, node: Archetype, componentId: i53)
 	local types = node.types
+	-- Component IDs are added incrementally, so inserting and sorting
+	-- them each time would be expensive. Instead this insertion sort can find the insertion 
+	-- point in the types array.
 	local at = findInsert(types, componentId)
 	if at == -1 then
+		-- If it finds a duplicate, it just means it is the same archetype so it can return it
+		-- directly instead of needing to hash types for a lookup to the archetype.
 		return node
 	end
 
@@ -245,6 +267,7 @@ end
 
 local function archetypeTraverseAdd(world: World, componentId: i53, from: Archetype): Archetype
 	if not from then 
+		-- If there was no source archetype then it should return the ROOT_ARCHETYPE
 		if not world.ROOT_ARCHETYPE then 
             local ROOT_ARCHETYPE = archetypeOf(world, {}, nil)
             world.ROOT_ARCHETYPE = ROOT_ARCHETYPE
@@ -254,6 +277,8 @@ local function archetypeTraverseAdd(world: World, componentId: i53, from: Archet
 	local edge = ensureEdge(from, componentId)
 
 	if not edge.add then
+		-- Save an edge using the component ID to the archetype to allow 
+		-- faster traversals to adjacent archetypes.
 		edge.add = findArchetypeWith(world, from, componentId)
 	end
 
@@ -270,26 +295,31 @@ end
 
 function World.set(world: World, entityId: i53, componentId: i53, data: unknown) 
 	local record = ensureRecord(world.entityIndex, entityId)
-	local sourceArchetype = record.archetype
+	local from = record.archetype
-	local destinationArchetype = archetypeTraverseAdd(world, componentId, sourceArchetype)
+	local to = archetypeTraverseAdd(world, componentId, from)
 
-	if sourceArchetype == destinationArchetype then 
+	if from == to then 
-		local archetypeRecord = destinationArchetype.records[componentId]
+		-- If the archetypes are the same it can avoid moving the entity 
-		destinationArchetype.columns[archetypeRecord][record.row] = data
+		-- and just set the data directly. 
+		local archetypeRecord = to.records[componentId]
+		from.columns[archetypeRecord][record.row] = data
+		-- Should fire an OnSet event here.
 		return
 	end
 
-	if sourceArchetype then
+	if from then
-		moveEntity(world.entityIndex, entityId, record, destinationArchetype)
+		-- If there was a previous archetype, then the entity needs to move the archetype 
+		moveEntity(world.entityIndex, entityId, record, to)
 	else
-		if #destinationArchetype.types > 0 then
+		if #to.types > 0 then
-			newEntity(entityId, record, destinationArchetype)
+			-- When there is no previous archetype it should create the archetype
-			onNotifyAdd(world, destinationArchetype, sourceArchetype, record.row, { componentId })
+			newEntity(entityId, record, to)
+			onNotifyAdd(world, to, from, record.row, { componentId })
 		end
 	end
 
-	local archetypeRecord = destinationArchetype.records[componentId]
+	local archetypeRecord = to.records[componentId]
-	destinationArchetype.columns[archetypeRecord][record.row] = data
+	to.columns[archetypeRecord][record.row] = data
 end
 
 local function archetypeTraverseRemove(world: World, componentId: i53, archetype: Archetype?): Archetype
@@ -316,9 +346,10 @@ function World.remove(world: World, entityId: i53, componentId: i53)
 	end
 end
 
+-- Keeping the function as small as possible to enable inlining
 local function get(componentIndex: { [i24]: ArchetypeMap }, record: Record, componentId: i24)
 	local archetype = record.archetype
-	local archetypeRecord = componentIndex[componentId].sparse[archetype.id]
+	local archetypeRecord = archetype.records[componentId]
 
 	if not archetypeRecord then
 		return nil
@@ -462,7 +493,7 @@ function World.query(world: World, ...: i53): Query
 			local entityId = archetype.entities[row :: number]
 			local columns = archetype.columns
 			local tr = compatibleArchetype[2]
-
+		
 			if queryLength == 1 then 
 				return entityId, columns[tr[1]][row]
 			elseif queryLength == 2 then 
@@ -528,7 +559,9 @@ end
 function World.component(world: World) 
 	local componentId = world.nextComponentId + 1	
 	if componentId > HI_COMPONENT_ID then 
-		error("Too many components")	
+		-- IDs are partitioned into ranges because component IDs are not nominal, 
+		-- so it needs to error when IDs intersect into the entity range.
+		error("Too many components, consider using world:entity() instead to create components.")	
 	end
 	world.nextComponentId = componentId
 	return componentId
@@ -539,6 +572,17 @@ function World.entity(world: World)
 	return world.nextEntityId + REST
 end
 
+function World.delete(world: World, entityId: i53) 
+	local entityIndex = world.entityIndex
+	local record = entityIndex[entityId]
+	moveEntity(entityIndex, entityId, record, world.ROOT_ARCHETYPE)
+	-- Since we just appended an entity to the ROOT_ARCHETYPE we have to remove it from
+	-- the entities array and delete the record. We know there won't be the hole since
+	-- we are always removing the last row.
+	--world.ROOT_ARCHETYPE.entities[record.row] = nil
+	--entityIndex[entityId] = nil
+end
+
 function World.observer(world: World, ...)
 	local componentIds = { ... }
 	

lib/init.spec.lua

@@ -191,5 +191,113 @@ return function()
 			end
 			expect(count).to.equal(1)
 		end)
+
+		it("should query all matching entities", function()
+
+			local world = jecs.World.new()
+			local A = world:component()
+			local B = world:component()
+
+			local entities = {}
+			for i = 1, N do
+				local id = world:entity()
+
+
+				world:set(id, A, true)
+				if i > 5 then world:set(id, B, true) end
+				entities[i] = id
+			end
+
+			for id in world:query(A) do
+				local i = table.find(entities, id)
+				expect(i).to.be.ok()
+				table.remove(entities, i)
+			end
+
+			expect(#entities).to.equal(0)
+		end)
+
+		it("should query all matching entities when irrelevant component is removed", function()
+
+			
+			local world = jecs.World.new()
+			local A = world:component()
+			local B = world:component()
+	
+			local entities = {}
+			for i = 1, N do
+				local id = world:entity()
+	
+				world:set(id, A, true)
+				world:set(id, B, true)
+				if i > 5 then world:remove(id, B, true) end
+				entities[i] = id
+			end
+	
+			local added = 0
+			for id in world:query(A) do
+				added += 1
+				local i = table.find(entities, id)
+				expect(i).to.be.ok()
+				table.remove(entities, i)
+			end
+	
+			expect(added).to.equal(N)
+		end)
+
+		it("should query all entities without B", function() 
+			local world = jecs.World.new()
+			local A = world:component()
+			local B = world:component()
+	
+			local entities = {}
+			for i = 1, N do
+				local id = world:entity()
+	
+				world:set(id, A, true)
+				if i < 5 then
+					entities[i] = id
+				else
+					world:set(id, B, true)
+				end
+				
+			end
+	
+			for id in world:query(A):without(B) do
+				local i = table.find(entities, id)
+				expect(i).to.be.ok()
+				table.remove(entities, i)
+			end
+	
+			expect(#entities).to.equal(0)
+		end)
+
+		it("should allow setting components in arbitrary order", function() 
+			local world = jecs.World.new()
+
+			local Health = world:entity()
+			local Poison = world:component()
+
+			local id = world:entity()
+			world:set(id, Poison, 5)
+			world:set(id, Health, 50)
+
+			expect(world:get(id, Poison)).to.equal(5)
+		end)
+
+		it("Should allow deleting components", function() 
+			local world = jecs.World.new()
+
+			local Health = world:entity()
+			local Poison = world:component()
+
+			local id = world:entity()
+			world:set(id, Poison, 5)
+			world:set(id, Health, 50)
+			world:delete(id)
+
+			expect(world:get(id, Poison)).to.never.be.ok()
+			expect(world:get(id, Health)).to.never.be.ok()
+		end)
 	end)
 end

tests/test1.lua

@@ -6,7 +6,6 @@ local TEST, CASE, CHECK, FINISH, SKIP = testkit.test()
 local N = 10
 
 TEST("world:query", function() 
-
     do CASE "should query all matching entities"
 
         local world = jecs.World.new()
@@ -83,6 +82,34 @@ TEST("world:query", function()
 
     end
 
+    do CASE "should allow setting components in arbitrary order" 
+        local world = jecs.World.new()
+
+        local Health = world:entity()
+        local Poison = world:component()
+
+        local id = world:entity()
+        world:set(id, Poison, 5)
+        world:set(id, Health, 50)
+
+        CHECK(world:get(id, Poison) == 5)
+    end
+
+    do CASE "Should allow deleting components" 
+        local world = jecs.World.new()
+
+        local Health = world:entity()
+        local Poison = world:component()
+
+        local id = world:entity()
+        world:set(id, Poison, 5)
+        world:set(id, Health, 50)
+        world:delete(id)
+
+        CHECK(world:get(id, Poison) == nil)
+        CHECK(world:get(id, Health) == nil)
+    end
+
 end)
 
 FINISH()