No damage effects on hp_max change

[appgurueu]

#10569 rebased and fixed. Closes #7876 - should this also alter the punch command being sent, to suppress the punch effect?

Test using the following code (EPILEPSY WARNING):

minetest.register_globalstep(function()
    for _, player in pairs(minetest.get_connected_players()) do
        local hp_max = math.random(1, 20)
        player:set_properties{hp_max = hp_max}
        player:set_hp(hp_max)
    end
end)

[Wuzzy2]

Will the damage effect also happen if hp_max increases? Because that shouldn’t happen.

[appgurueu]

Will the damage effect also happen if hp_max increases? Because that shouldn’t happen.

A increase of hp_max has no effect, as the HP stays the same. Only a decrease used to trigger a damage flash, as the reduced HP had to be sent, which is what this PR fixes.

[ghost]

Could you rebase this on master and test on your end? It's no longer possible to compile this on the latest IrrlichtMT, and after performing a local rebase I saw incorrect results from a quick test (no effect on fall damage, effect present on max hp changes). To me, that implies either this doesn't function as advertised, or (more likely) something relevant has changed in the interim that needs addressing.

[ghost]

Tested again, still seeing incorrect results. First of all, just changing max hp still seems to trigger a damage flash. Additionally, fall damage no longer causes a flash. To help you out a bit, I did some digging for the latter. Fall damage in singleplayer is triggering ClientEnvironment::damageLocalPlayer, which does not set the effect field in its damage event. While this damage does pass through Client::handleCommand_HP which can send an event with the flag set, at that point the player's HP has already changed so no event is sent. Thus, any damage received via ClientEnvironment::damageLocalPlayer no longer triggers an effect even though it should.

I should not that I'm testing in singleplayer, so if you've been using a separate server to test this may be the source of your bugs.

[appgurueu]

Tested again, still seeing incorrect results. First of all, just changing max hp still seems to trigger a damage flash. Additionally, fall damage no longer causes a flash. To help you out a bit, I did some digging for the latter. [...]

Thanks, you were correct with your digging. Fixed.

[ghost]

Thanks for the quick turnaround. I'm too tired for a full test tonight, but I can at least confirm that in singleplayer fall damage is displaying the proper effect again, and max hp changes are not. So far so good.

I'll try to give this a more thorough test and review by the end of the week.

[ghost]

Tested the following in an actual game context, using Mineclone 2 and an unreleased game prototype:

• node damage_per_second: Effect
• PVP damage: Effect
• non-PVP combat damage (ie. getting punched by a mob): Effect
• Drowning damage: Effect
• Fall damage: Effect
• Max HP reduction from a status effect wearing out: No Effect

I tested all of these in singleplayer, locally-hosted multiplayer as host, and locally-hosted multiplayer as a client and got consistent results in each test. I also tested locally-hosted multiplayer with an unpatched host and with an unpatched client. In both cases the bug persisted (as expected), but no errors occurred due to the packet changes.

I'm satisfied with this result, and while I think you could improve the name of effect the code is still readable as-is so I'll leave that to your discretion and approve now.

[ghost]

I think this could stand to be more explicit. Something like show_effect would make the purpose clearer at a glance imo.

[appgurueu]

Might call this something like silent, quiet, hidden or the like if inverted, but show_effect may be slightly misleading as setting this to false will also suppress the audio effect.

[ghost]

I'd be fine with those options as well.

[sfan5]

Not tested but lgtm