Allow detection of damage greater than HP (#15160)

Co-authored-by: Gregor Parzefall <gregor.parzefall@posteo.de>

doc/lua_api.md

@@ -5850,8 +5850,13 @@ Call these functions only at load time!
     * `clicker`: ObjectRef - Object that acted upon `player`, may or may not be a player
 * `minetest.register_on_player_hpchange(function(player, hp_change, reason), modifier)`
     * Called when the player gets damaged or healed
+    * When `hp == 0`, damage doesn't trigger this callback.
+    * When `hp == hp_max`, healing does still trigger this callback.
     * `player`: ObjectRef of the player
     * `hp_change`: the amount of change. Negative when it is damage.
+      * Historically, the new HP value was clamped to [0, 65535] before
+        calculating the HP change. This clamping has been removed as of
+        Minetest 5.10.0
     * `reason`: a PlayerHPChangeReason table.
         * The `type` field will have one of the following values:
             * `set_hp`: A mod or the engine called `set_hp` without

games/devtest/mods/unittests/player.lua

@@ -42,41 +42,97 @@ unittests.register("test_hpchangereason", run_hpchangereason_tests, {player=true
 --
 
 local expected_diff = nil
+local hpchange_counter = 0
+local die_counter = 0
 core.register_on_player_hpchange(function(player, hp_change, reason)
 	if expected_diff then
 		assert(hp_change == expected_diff)
+		hpchange_counter = hpchange_counter + 1
 	end
 end)
+core.register_on_dieplayer(function()
+	die_counter = die_counter + 1
+end)
+
+local function hp_diference_test(player, hp_max)
+	assert(hp_max >= 22)
 
-local function run_hp_difference_tests(player)
 	local old_hp = player:get_hp()
 	local old_hp_max = player:get_properties().hp_max
 
-	expected_diff = nil
-	player:set_properties({hp_max = 30})
-	player:set_hp(22)
+	hpchange_counter = 0
+	die_counter = 0
 
-	-- final HP value is clamped to >= 0 before difference calculation
-	expected_diff = -22
+	expected_diff = nil
+	player:set_properties({hp_max = hp_max})
+	player:set_hp(22)
+	assert(player:get_hp() == 22)
+	assert(hpchange_counter == 0)
+	assert(die_counter == 0)
+
+	-- HP difference is not clamped
+	expected_diff = -25
 	player:set_hp(-3)
-	-- and actual final HP value is clamped to >= 0 too
+	-- actual final HP value is clamped to >= 0
 	assert(player:get_hp() == 0)
+	assert(hpchange_counter == 1)
+	assert(die_counter == 1)
 
 	expected_diff = 22
 	player:set_hp(22)
 	assert(player:get_hp() == 22)
+	assert(hpchange_counter == 2)
+	assert(die_counter == 1)
 
-	-- final HP value is clamped to <= U16_MAX before difference calculation
-	expected_diff = 65535 - 22
+	-- Integer overflow is prevented
+	-- so result is S32_MIN, not S32_MIN - 22
+	expected_diff = -2147483648
+	player:set_hp(-2147483648)
+	-- actual final HP value is clamped to >= 0
+	assert(player:get_hp() == 0)
+	assert(hpchange_counter == 3)
+	assert(die_counter == 2)
+
+	-- Damage is ignored if player is already dead (hp == 0)
+	expected_diff = "never equal"
+	player:set_hp(-11)
+	assert(player:get_hp() == 0)
+	-- no on_player_hpchange or on_dieplayer call expected
+	assert(hpchange_counter == 3)
+	assert(die_counter == 2)
+
+	expected_diff = 11
+	player:set_hp(11)
+	assert(player:get_hp() == 11)
+	assert(hpchange_counter == 4)
+	assert(die_counter == 2)
+
+	-- HP difference is not clamped
+	expected_diff = 1000000 - 11
 	player:set_hp(1000000)
-	-- and actual final HP value is clamped to <= hp_max
-	assert(player:get_hp() == 30)
+	-- actual final HP value is clamped to <= hp_max
+	assert(player:get_hp() == hp_max)
+	assert(hpchange_counter == 5)
+	assert(die_counter == 2)
+
+	-- "Healing" is not ignored when hp == hp_max
+	expected_diff = 80000 - hp_max
+	player:set_hp(80000)
+	assert(player:get_hp() == hp_max)
+	-- on_player_hpchange_call expected
+	assert(hpchange_counter == 6)
+	assert(die_counter == 2)
 
 	expected_diff = nil
 	player:set_properties({hp_max = old_hp_max})
 	player:set_hp(old_hp)
 	core.close_formspec(player:get_player_name(), "") -- hide death screen
 end
+local function run_hp_difference_tests(player)
+	hp_diference_test(player, 22)
+	hp_diference_test(player, 30)
+	hp_diference_test(player, 65535) -- U16_MAX
+end
 unittests.register("test_hp_difference", run_hp_difference_tests, {player=true})
 
 --

src/server/player_sao.cpp

@@ -519,12 +519,13 @@ void PlayerSAO::rightClick(ServerActiveObject *clicker)
 
 void PlayerSAO::setHP(s32 target_hp, const PlayerHPChangeReason &reason, bool from_client)
 {
-	target_hp = rangelim(target_hp, 0, U16_MAX);
-
-	if (target_hp == m_hp)
+	if (target_hp == m_hp || (m_hp == 0 && target_hp < 0))
 		return; // Nothing to do
 
-	s32 hp_change = m_env->getScriptIface()->on_player_hpchange(this, target_hp - (s32)m_hp, reason);
+	// Protect against overflow.
+	s32 hp_change = std::max<s64>((s64)target_hp - (s64)m_hp, S32_MIN);
+
+	hp_change = m_env->getScriptIface()->on_player_hpchange(this, hp_change, reason);
 	hp_change = std::min<s32>(hp_change, U16_MAX); // Protect against overflow
 
 	s32 hp = (s32)m_hp + hp_change;