Unify health vars at /obj level #3524

PsyCommando · 2023-12-11T23:54:50Z

Description of changes

Just the first step in the pretty elaborate health/damage unification I'm currently sorting out from those old attempts I made.

Ideally it would go something like that:

Unify health vars at /obj level
- health/max_health
- hitsound
- armor stuff
- handling for a "destroyed state". Or maybe replacing with a wreck object for things that got a "destroyed state"
- temperature damage values
Unify damage procs at /obj level
- take_damage proc
- healing procs
- damage destruction types procs
- damage hit effects procs
- health check procs
- damage mitigation/armor procs
- material health setup proc (this one is going to be extra annoying, and would wait on a material unification)
Unify attack procs at /obj level
- unarmed attack procs
- weapon attack procs
- throw attack procs
- projectile attack procs
- explosion attack procs
- act procs
Unify attack at /atom level. (Mobs and objects have a really different call chain for damage procs. But they could definitely be generalized and abstracted to work on both.)
- Simplify and share a mostly similar call chain for taking damage.
  - Abstract locational damage to the atom level. (defense zones would only work for things that implement them, like mobs)
  - Abstract damaging health handling, so if something must keep track of damage types and amounts received they can do that.
  - Make melee damage call chain similar for both obj and mobs
  - Make projectile damage call chain similar for both obj and mobs

And then after all that stuff is worked out, I'm planning on doing some general QoL stuff:

datumify damage types, and cut on a lot of repetitive damage type handling down the chain. Like for wounds, effects, destruction effects, dismembering, bleeding, lore description, detectability, properties, etc..
Find some way to cut down on parameter bloat/mismatching for damage handling procs. Like maybe having some sort of /datum/dealt_damage passed down or something? A bit like how thrown things are handled currently. I just gotta figure if it would cause a serious problem when a lot of damage is being dealt, and it's creating a lot of datums.. Especially since a lot of them would be mostly similar besides maybe the damage value...

Thoughts and suggestions are welcome!

Changelog

🆑
refactor: Moved all health vars below /obj back to /obj level.
/:cl:

Having it in atom_temperature is really annoying when you're trying to find it.

comma · 2023-12-12T09:34:02Z

code/game/objects/structures/__structure.dm

@@ -40,15 +39,15 @@
 		fluid_update(TRUE)

 /obj/structure/get_examined_damage_string(health_ratio)
-	if(maxhealth == -1)
+	if(max_health == -1)


ITEM_HEALTH_NO_DAMAGE? Or even can_take_damage()

I'd rather keep the focus to moving the vars in this PR. I don't want to give the impression that structures actually like are guaranteed to handle that special value at all, cause they don't yet. Most of their damage handling is tailor made.

This override can actually be deleted entirely as it is duplicated logic from the /obj proc. I agree with chinsky that the other overrides should use ITEM_HEALTH_NO_DAMAGE as the base /obj proc does identical checks using it.

Further note, can_take_damage() and such use the define, so it looks like at a fundamental level structures do handle the special value unless I'm misunderstanding.

They all can, and they will use a getter and use the proper overridable procs.

I'm trying to make bite-sized PRs that don't fundamentally change the existing logic, to reduce conflicts and to make them faster to review. Also, if I start messing with that, I know I'm gonna start changing way too many things XD

Honestly, I probably should have left the procs out for this PR.. I could cherry pick the commit out of this PR.

MistakeNot4892

Not a blocker, but I would have preferred to replace all the max_health uses with a getter I think (allows for injecting modifiers etc).

Generally seems fine other than the noted potential issue with ITEM_HEALTH_NO_DAMAGE and the damage string overrides. It might be worth just making it use can_be_damaged() instead of checking ITEM_HEALTH_NO_DAMAGE.

PsyCommando · 2023-12-13T23:17:22Z

Not a blocker, but I would have preferred to replace all the max_health uses with a getter I think (allows for injecting modifiers etc).

Generally seems fine other than the noted potential issue with ITEM_HEALTH_NO_DAMAGE and the damage string overrides. It might be worth just making it use can_be_damaged() instead of checking ITEM_HEALTH_NO_DAMAGE.

That's all on the roadmap. I just wanted to keep changing var names separate since it causes a lot of diff/conflicts. I'm considering handling renaming of procs/vars as their own PR to try to help with that.

MistakeNot4892 · 2023-12-13T23:22:42Z

I don't think this PR is large enough to warrant splitting out cleanup of procs, honestly.

PsyCommando · 2023-12-13T23:27:24Z

I don't think this PR is large enough to warrant splitting out cleanup of procs, honestly.

Well, on the previous attempt I made, you told me the PR was too big to review. So, I'm anticipating that and just chopping it all down. And, I need to change a lot of the damage logic currently in place to unify things.

It's up to you really, I don't mind just doing it all in a single PR again. But I'd be a bit disappointed if you'd tell me it's too big and to split it down afterwards.

MistakeNot4892 · 2023-12-14T00:38:57Z

We're talking about a 300 line PR that might have another 20 lines added by cleaning up a specific proc. :P The previous PRs were hundreds or thousands of lines across multiple functional changes.

PsyCommando · 2023-12-14T19:39:43Z

I don't think this PR is large enough to warrant splitting out cleanup of procs, honestly.

Fair enough. I made the change at the /obj level since there's not really any reason to show the damage desc for anything invincible.

MistakeNot4892 · 2023-12-15T03:45:38Z

Thanks, sorry for being pushy about it. Just have a bad feeling that fixing it down the track will potentially get overlooked.

MistakeNot4892 · 2023-12-15T03:47:15Z

code/game/objects/objs.dm

+
+/obj/Initialize(mapload)
+	//Health should be set to max_health only if it's null.
+	if(isnull(health))


Non-blocking side-thought, but this could potentially be done at the end of init, and get_max_health() or such could wrap in things like material durability influencing max HP.

Yeah, but that's voluntary. Some things do not start at full health, like vines, blobs, and a few other things. So you don't want to automatically set those to max_health. And if you'd do the same check inside a material update proc, you'd have to have some way of knowing if you can mess with the health value or not since material could change at runtime as well as being set during init.
It also makes my life much easier downstream when loading from save something with a saved health.

I'm not really thrilled about making a get_max_health proc that would return something that may change over time. Because that means having to figure out when that value change and update health accordingly. Like material durability could easily just be calculated into max_health as it is. And it would be easier to keep a coherent internal state between changes to health values.

Currently, only at the /obj level, I'm gating health and max_health changes behind set_max_health and set_health. And set_max_health has a flag for telling how to handle updating the current health so it's a bit more flexible (for now those flags are: adding the difference to health, leaving health alone, reseting health to max_health, or just clamping health to max_health).

Also, max_health is going to be accessed a lot, and using a proc might have an overhead. Especially for things that call health procs in a loop, or through subsystems.
And, not all atoms really have a concept of health. And usually you can easily replace most things that would needs to access get_max_health values with something getting a percentage of damage/health, which furthers makes it easier to abstract things out.

code/game/objects/objs.dm

MistakeNot4892 · 2023-12-15T08:46:59Z

code/modules/shieldgen/emergency_shield.dm

 	var/shield_generate_power = 7500	//how much power we use when regenerating
 	var/shield_idle_power = 1500		//how much power we use when just being sustained.

 /obj/machinery/shield/malfai
 	name = "emergency forcefield"
 	desc = "A weak forcefield which seems to be projected by the emergency atmosphere containment field."
-	health = max_health/2 // Half health, it's not suposed to resist much.
+	health = 100 // Half health, it's not suposed to resist much.


This should be max_health = 100 right?

You're right! gonna fix that

PsyCommando · 2023-12-16T00:57:32Z

Random unit test failure over 9 created /obj/item instances failing to gc apparently..

PsyCommando added 3 commits December 11, 2023 17:39

Move obj/Initialize back to obj.dm

6fceae2

Having it in atom_temperature is really annoying when you're trying to find it.

Moved health vars to /obj level

1027d1c

Moved basic damage procs to obj level

232e2c9

comma reviewed Dec 12, 2023

View reviewed changes

MistakeNot4892 requested changes Dec 13, 2023

View reviewed changes

MistakeNot4892 added the awaiting author This PR is awaiting action from the author before it can be merged. label Dec 13, 2023

prevent health desc showing on invincible atom

715f808

PsyCommando requested a review from MistakeNot4892 December 14, 2023 19:39

MistakeNot4892 reviewed Dec 15, 2023

View reviewed changes

code/game/objects/objs.dm Show resolved Hide resolved

MistakeNot4892 added ready for review This PR is ready for review and merge. and removed awaiting author This PR is awaiting action from the author before it can be merged. labels Dec 15, 2023

MistakeNot4892 reviewed Dec 15, 2023

View reviewed changes

Fix setting health instead of max_health

aa51612

MistakeNot4892 approved these changes Dec 16, 2023

View reviewed changes

comma merged commit cffaec7 into NebulaSS13:dev Dec 16, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify health vars at /obj level #3524

Unify health vars at /obj level #3524

PsyCommando commented Dec 11, 2023

comma Dec 12, 2023 •

edited

Loading

PsyCommando Dec 12, 2023

MistakeNot4892 Dec 13, 2023

MistakeNot4892 Dec 13, 2023

PsyCommando Dec 13, 2023

MistakeNot4892 left a comment

PsyCommando commented Dec 13, 2023

MistakeNot4892 commented Dec 13, 2023

PsyCommando commented Dec 13, 2023

MistakeNot4892 commented Dec 14, 2023

PsyCommando commented Dec 14, 2023

MistakeNot4892 commented Dec 15, 2023

MistakeNot4892 Dec 15, 2023

PsyCommando Dec 15, 2023

MistakeNot4892 Dec 15, 2023

PsyCommando Dec 15, 2023

PsyCommando Dec 15, 2023

PsyCommando commented Dec 16, 2023

Unify health vars at /obj level #3524

Unify health vars at /obj level #3524

Conversation

PsyCommando commented Dec 11, 2023

Description of changes

Changelog

comma Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MistakeNot4892 left a comment

Choose a reason for hiding this comment

PsyCommando commented Dec 13, 2023

MistakeNot4892 commented Dec 13, 2023

PsyCommando commented Dec 13, 2023

MistakeNot4892 commented Dec 14, 2023

PsyCommando commented Dec 14, 2023

MistakeNot4892 commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PsyCommando commented Dec 16, 2023

comma Dec 12, 2023 •

edited

Loading