Add delete test

1. Ensures that monoids are registered correctly
2. Type checks on effect changes
3. Consistent use of object-oriented functions (':_get_player_data', ...)
4. 'branch:delete(...)' now also returns 'true' on success

The code still has inconsistent variable naming in several places, which
should be addressed later for easier understanding.

.luacheckrc

@@ -3,9 +3,12 @@
 ignore = { "212" }
 
 read_globals = {
+	table = {fields = {"copy"}},
+
 	"core",
 	"minetest",
 	"vector",
+	"dump",
 }
 
 globals = {

init.lua

@@ -1,6 +1,8 @@
 local modpath = minetest.get_modpath(minetest.get_current_modname()) .. "/"
 
-player_monoids = {}
+player_monoids = {
+	api_version = 2
+}
 
 local mon_meta = {}
 mon_meta.__index = mon_meta
@@ -11,50 +13,39 @@ local nop = function() end
 --   def: The monoid definition.
 --   player_map: A map from player names to their branch maps. Branch maps
 --     contain branches, and each branch holds an 'effects' table.
---   value_cache: A map from player names to the cached value for the monoid.
 --   next_id: The next unique ID to assign an effect.
 
---[[
-In def, you can optionally define:
-
-  - apply(new_value, player)
-  - on_change(old_value, new_value, player, branch)
-  - listen_to_all_changes (bool)
-  - on_branch_created(monoid, player, branch_name)
-  - on_branch_deleted(monoid, player, branch_name)
-
-These hooks allow you to respond to monoid changes, branch creation, and branch deletion.
-]]
-
 player_monoids.make_monoid = function(def)
-	local mon = {}
+	assert(type(def) == "table")
 
 	-- Clone the definition to avoid mutating the original
-	local actual_def = {}
+	local actual_def = {
+		-- Default values of optional fields
+		apply = nop,
+		on_change = nop,
+		on_branch_created = nop,
+		on_branch_deleted = nop,
+		listen_to_all_changes = false,
+	}
 	for k, v in pairs(def) do
 		actual_def[k] = v
 	end
 
-	if not actual_def.apply then
+	-- Mandatory fields
-		actual_def.apply = nop
+	assert(actual_def.identity ~= nil)
-	end
+	-- (combine is unused)
-	if not actual_def.on_change then
+	assert(type(actual_def.fold) == "function")
-		actual_def.on_change = nop
+	assert(type(actual_def.apply) == "function")
-	end
-	if not actual_def.on_branch_created then
-		actual_def.on_branch_created = nop
-	end
-	if not actual_def.on_branch_deleted then
-		actual_def.on_branch_deleted = nop
-	end
-	if actual_def.listen_to_all_changes == nil then
-		actual_def.listen_to_all_changes = false
-	end
 
+	-- Optional fields
+	assert(type(actual_def.on_change) == "function")
+	assert(type(actual_def.listen_to_all_changes) == "boolean")
+	assert(type(actual_def.on_branch_created) == "function")
+	assert(type(actual_def.on_branch_deleted) == "function")
+
+	local mon = {}
 	mon.def = actual_def
-
+	mon.player_map = {} -- Contains the branch data
-	mon.player_map = {} -- p_name -> { active_branch="main", branches={ branch_name={ effects={}, value=...} } }
-	mon.value_cache = {} -- p_name -> numeric or table
 	mon.next_id = 1
 
 	setmetatable(mon, mon_meta)
@@ -63,51 +54,60 @@ player_monoids.make_monoid = function(def)
 	minetest.register_on_leaveplayer(function(player)
 		local p_name = player:get_player_name()
 		mon.player_map[p_name] = nil
-		mon.value_cache[p_name] = nil
 	end)
 
-	-- Initialize branches for the monoid
-	function mon:init_branches(player_name)
-		self.player_map[player_name] = {
-			active_branch = "main",
-			branches = {
-				main = {
-					effects = {},
-					value = def.identity
-				}
-			}
-		}
-	end
-
 	return mon
 end
 
-local function init_player_branches_if_missing(self, p_name)
+
-	if not self.player_map[p_name] then
+--- @brief Gets or initializes the player data of the current monoid
-		self:init_branches(p_name)
+--- @param p_name     string
+--- @param do_reset   boolean, whether to reset all player data. Default: false
+--- @return A table, the player data.
+function mon_meta:_get_player_data(p_name, do_reset)
+	local p_data
+	if not do_reset then
+		p_data = self.player_map[p_name]
+		if p_data then
+			return p_data
 		end
 	end
 
--- Create or return existing branch. If a new one is created, fire on_branch_created.
+	p_data = {
-local function get_or_create_branch_data(self, p_name, branch_name)
+		active_branch = "main",
-	local branches = self.player_map[p_name].branches
+		branches = {}
-	local existing_branch = branches[branch_name]
-
-	if not existing_branch then
-		branches[branch_name] = {
-			effects = {},
-			value = self.def.identity
 	}
+	self.player_map[p_name] = p_data
 
-		existing_branch = branches[branch_name]
+	-- Create the main branch
+	local bdata = self:_get_branch_data(p_name, "main")
+	p_data.last_value = bdata.value -- for 'on_change'
 
-		local player = minetest.get_player_by_name(p_name)
+	return p_data
+end
+
+--- @brief Gets or initializes the givne branch
+function mon_meta:_get_branch_data(p_name, branch_name)
+	local branches = self.player_map[p_name].branches
+	local branch = branches[branch_name]
+	if branch then
+		return branch
+	end
+
+	-- Create
+	branch = {
+		effects = {},
+		value = table.copy(self.def.identity)
+	}
+	branches[branch_name] = branch
+
+	if branch_name ~= "main" then
+		local player = core.get_player_by_name(p_name)
 		if player then
 			self.def.on_branch_created(self, player, branch_name)
 		end
 	end
-
+	return branch
-	return existing_branch
 end
 
 -- decide if to call on_change for this change based on listen_to_all_changes
@@ -118,62 +118,67 @@ function mon_meta:call_on_change(old_value, new_value, player, branch_name)
 	end
 end
 
-function mon_meta:add_change(player, value, id, branch_name)
+--- @brief Internal function to change (or remove) an effect
+--- @param player       ObjectRef
+--- @param value        new effect value (may be nil)
+--- @param id           string/integer, effect ID
+--- @param branch_name  string, branch to modify
+function mon_meta:_set_change(player, value, id, branch_name)
+	assert(value == nil or type(value) == type(self.def.identity))
+
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	-- TODO: 'new_branch' and 'checkout_branch' should be used instead to create the branch!
-
+	local p_branch_data = self:_get_branch_data(p_name, branch_name)
-	local branch = branch_name or "main"
-	local p_branch_data = get_or_create_branch_data(self, p_name, branch)
-
-	local p_effects = p_branch_data.effects
-
-	local actual_id = id or self.next_id
-	if not id then
-		self.next_id = actual_id + 1
-	end
 
 	local old_total = p_branch_data.value
-	p_effects[actual_id] = value
 
-	local new_total = self.def.fold(p_effects)
+	p_branch_data.effects[id] = value
+	local new_total = self.def.fold(p_branch_data.effects)
 	p_branch_data.value = new_total
 
-	if self.player_map[p_name].active_branch == branch then
+	if self.player_map[p_name].active_branch == branch_name then
 		self.def.apply(new_total, player)
 	end
 
-	self:call_on_change(old_total, new_total, player, branch)
+	self:call_on_change(old_total, new_total, player, branch_name)
-	return actual_id
+end
+
+function mon_meta:add_change(player, value, id, branch_name)
+	local p_name = player:get_player_name()
+	branch_name = branch_name or "main"
+
+	-- Create if not existing
+	self:_get_player_data(p_name)
+	self:_get_branch_data(p_name, branch_name)
+
+	if not id then
+		id = self.next_id
+		self.next_id = self.next_id + 1
+	end
+
+	self:_set_change(player, value, id, branch_name)
+	return id
 end
 
 function mon_meta:del_change(player, id, branch_name)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	local p_data = self:_get_player_data(p_name)
 
-	local branch = branch_name or "main"
+	branch_name = branch_name or "main"
-	local p_branch_data = get_or_create_branch_data(self, p_name, branch)
+	local p_branch_data = p_data.branches[branch_name]
-	if not p_branch_data then return end
+	if not p_branch_data then
-
+		return
-	local p_effects = p_branch_data.effects
-	local old_total = p_branch_data.value
-
-	p_effects[id] = nil
-	local new_total = self.def.fold(p_effects)
-	p_branch_data.value = new_total
-
-	if self.player_map[p_name].active_branch == branch then
-		self.def.apply(new_total, player)
 	end
 
-	self:call_on_change(old_total, new_total, player, branch)
+	self:_set_change(player, nil, id, branch_name)
 end
 
 function mon_meta:reset_branch(player, branch_name)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	local p_data = self:_get_player_data(p_name)
 
-	local branch = branch_name or "main"
+	branch_name = branch_name or "main"
-	local bdata = self.player_map[p_name].branches[branch]
+	local bdata = p_data.branches[branch_name]
 	if not bdata then
 		return -- Branch doesn't exist, nothing to reset
 	end
@@ -182,25 +187,27 @@ function mon_meta:reset_branch(player, branch_name)
 
 	-- Clear effects and recalc
 	bdata.effects = {}
-	local new_total = self.def.fold({})
+	local new_total = table.copy(self.identity)
 	bdata.value = new_total
 
-	-- Update active branch
+	local active_branch = p_data.active_branch or "main"
-	local active_branch = self.player_map[p_name].active_branch or "main"
+	if branch_name == active_branch then
-	local active_branch_data = self.player_map[p_name].branches[active_branch]
+		-- Apply the new values
-	self.value_cache[p_name] = active_branch_data.value
+		p_data.last_value = bdata.value
-	self.def.apply(active_branch_data.value, player)
+		self.def.apply(bdata.value, player)
-
-	-- Fire on_change for the branch being reset
-	self:call_on_change(old_total, new_total, player, branch)
 	end
 
--- new method: create a branch for a player, but do NOT check it out
+	-- Fire on_change for the branch being reset
+	self:call_on_change(old_total, new_total, player, branch_name)
+end
+
+-- Create a branch for a player, but do NOT check it out
 function mon_meta:new_branch(player, branch_name)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
 
-	get_or_create_branch_data(self, p_name, branch_name)
+	-- Create if not existing
+	self:_get_player_data(p_name)
+	self:_get_branch_data(p_name, branch_name)
 
 	return self:get_branch(branch_name)
 end
@@ -228,32 +235,7 @@ function mon_meta:get_branch(branch_name)
 			return branch_name
 		end,
 		delete = function(_, player)
-			local p_name = player:get_player_name()
+			return monoid:delete_branch(player, branch_name)
-			init_player_branches_if_missing(monoid, p_name)
-
-			local player_data = monoid.player_map[p_name]
-			if not player_data then
-				return
-			end
-
-			local existing_branch = player_data.branches[branch_name]
-			if not existing_branch or branch_name == "main" then
-				return
-			end
-
-			-- If it's the active branch, switch to main
-			if player_data.active_branch == branch_name then
-				player_data.active_branch = "main"
-				local new_main_total = monoid:value(player, "main")
-				monoid.value_cache[p_name] = new_main_total
-
-				monoid.def.apply(new_main_total, player)
-			end
-
-			-- Remove the branch
-			player_data.branches[branch_name] = nil
-
-			monoid.def.on_branch_deleted(monoid, player, branch_name)
 		end,
 	}
 end
@@ -266,62 +248,71 @@ end
 
 function mon_meta:get_branches(player)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	local p_data = self:_get_player_data(p_name)
 
-	local branch_map = self.player_map[p_name].branches or {}
 	local result = {}
-	for b_name, _ in pairs(branch_map) do
+	for b_name, _ in pairs(p_data.branches) do
 		result[b_name] = self:get_branch(b_name)
 	end
 	return result
 end
 
 function mon_meta:delete_branch(player, branch_name)
-	local b = self:get_branch(branch_name)
+	local p_name = player:get_player_name()
+	local player_data = self:_get_player_data(p_name)
 
-	if not b then
+	local existing_branch = player_data.branches[branch_name]
+	if not existing_branch or branch_name == "main" then
 		return false
 	end
 
-	b:delete(player)
+	-- If it's the active branch, switch to main
+	if player_data.active_branch == branch_name then
+		player_data.active_branch = "main"
+		local new_main_total = self:value(player, "main")
+
+		player_data.last_value = new_main_total
+		self.def.apply(new_main_total, player)
+	end
+
+	-- Remove the branch
+	player_data.branches[branch_name] = nil
+
+	self.def.on_branch_deleted(self, player, branch_name)
+	return true
 end
 
 minetest.register_on_joinplayer(function(player)
-	for _, monoid_instance in pairs(player_monoids) do
+	local p_name = player:get_player_name()
-		if type(monoid_instance) == "table" and monoid_instance.init_branches then
+	for _, monoid in pairs(player_monoids) do
-			monoid_instance:init_branches(player:get_player_name())
+		if type(monoid) == "table" and monoid._get_player_data then
+			monoid:_get_player_data(p_name)
 		end
 	end
 end)
 
 function mon_meta:value(player, branch_name)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	local p_data = self:_get_player_data(p_name)
 
-	local chosen_branch = branch_name or self.player_map[p_name].active_branch or "main"
+	branch_name = branch_name or p_data.active_branch
-	local p_data = self.player_map[p_name]
+	local bdata = p_data.branches[branch_name]
-	local bdata = p_data.branches[chosen_branch]
+	return bdata and bdata.value or table.copy(self.def.identity)
-	if not bdata then
-		return self.def.identity
-	end
-
-	local calculated_value = self.def.fold(bdata.effects)
-	return calculated_value
 end
 
 function mon_meta:checkout_branch(player, branch_name)
 	local p_name = player:get_player_name()
-	init_player_branches_if_missing(self, p_name)
+	local p_data = self:_get_player_data(p_name)
 
-	local old_total = self.value_cache[p_name] or self.def.identity
+	local old_total = p_data.last_value
 	local checkout_branch = self:new_branch(player, branch_name)
 
-	self.player_map[p_name].active_branch = branch_name
+	p_data.active_branch = branch_name
 	local new_total = self:value(player)
-	self.value_cache[p_name] = new_total
 
-	self:call_on_change(old_total, new_total, player, branch_name)
+	p_data.last_value = new_total
 	self.def.apply(new_total, player)
+	self:call_on_change(old_total, new_total, player, branch_name)
 
 	return checkout_branch
 end

test.lua

@@ -424,12 +424,16 @@ local function test_on_branch_create_delete(player)
 	end
 
 	local del_branch = speed:checkout_branch(player, "my_del_branch")
+	local retval
 	if not del_branch then
 		minetest.chat_send_player(p_name, "[OnBranchCreateDelete] FAIL: couldn't create 'my_del_branch'.")
 	else
 		minetest.chat_send_player(p_name, "[OnBranchCreateDelete] Created 'my_del_branch'. Deleting...")
-		del_branch:delete(player)
+		retval = del_branch:delete(player)
+		minetest.chat_send_player(p_name, ((retval == true) and "PASS" or "FAIL") .. " - branch:delete(...) #1")
 	end
+	retval = del_branch:delete(player)
+	minetest.chat_send_player(p_name, ((retval == false) and "PASS" or "FAIL") .. " - branch:delete(...) #2")
 
 	if created_count == 0 then
 		minetest.chat_send_player(p_name, "[OnBranchCreateDelete] FAIL: on_branch_created not called.")