Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Damage handlers. #3418

Closed
wants to merge 1 commit into from
Closed

Conversation

MistakeNot4892
Copy link
Contributor

@MistakeNot4892 MistakeNot4892 commented Oct 9, 2023

Another fey mood like the config system. Opening so I don't forget about it.

Description of changes

  • Replaces string-based damage types with a decl system.
  • Unifies the various damage procs under take_damage() and heal_damage() which in turn hook into the decl damage handler system.
  • Various other associated changes.

Why and what will this PR improve

Cleaner code, better modpack support for new damage types.

TODO

  • Basic conversion/unification.
  • Reimplement robo_repair and override_droplimb passing.
  • Look at converting the various /obj/effect subtypes with bespoke health handling.
  • Add damage immunity and restrict atoms to only suffering from brute and burn by default.
  • Add multi-damage-type proc to avoid repeated update health etc.
  • Test and make sure it all actually works.
  • Test exosuit damage.
  • Test mech component damage.
  • Test human damage.
  • Test robot damage.
  • Test drone damage.
  • Test AI damage.
  • Test simple animal damage.
  • Test slime damage.
  • Test blob damage.
  • Test structure damage.
  • Test wall damage.
  • Test machinery damage.
  • Test shield damage.
  • Maybe do a decl system for wound types as well.

Authorship

Myself.

Changelog

🆑
tweak: The way damage is handled has been drastically rewritten across the board. Please report any bugs.
/:cl:

@MistakeNot4892 MistakeNot4892 added the work in progress This PR is under development and shouldn't be merged. label Oct 9, 2023
@MistakeNot4892 MistakeNot4892 force-pushed the damage branch 2 times, most recently from 8ace93d to 9474a6b Compare October 10, 2023 01:42
@PsyCommando
Copy link
Contributor

So, I got a few thoughts after reading the code, and from what I think you're trying to do.

Mainly, I feel like the damage handlers being stateless decl that handle damage via many highly specialized procs is maybe not the best way to go about things?

A few problems come to mind:

  • Adding/removing damage types would involve a lot of work across a lot of atoms, since handlers have specialized procs for many different specialized cases?
  • Adding/removing specialized handling for a given atom type would involve adding specialized procs to all the damage handler objects, and adding each times extra use cases to validate for every atoms?
  • Another problem is the way damage handlers are overridden via list? Creating multiple list instances like that would probably be a bad thing to scale up to the atom level?
  • The advantages of being able to re-use the same damage handler decls across atom types seems to be not really worth it? Proc inheritance would be sufficient in pretty much the vast majority of cases? And having to create and maintain a ton of damage handlers implementations with slight variations would make everyone's lives much harder imo?

I feel like, damage handling could be a lot less complicated by just using inheritance and sensibly dividing damage handling into inherited procs, and some damage type definition /decl?
The damage definition datums would contain all the info you'd need for the base procs to handle damage in a generic way, and then any extra handling could be done in a number of ways, including overriding inherited procs, possibly extra fields/proc on the damage type decl, or even just using callbacks?

Also, another thing is how to deal deal with self/internal-damage, and raw damage with this setup? Since those should probably not involve armor checks, physical wounds creation, and like flavor text/sounds/visual effects, but generally still go through the same procs for validation and etc.
Also, it might be desirable to handle some things considered damages currently separately? Like oxygen/toxin damage which are organ exclusive mechanics and so on.

I might have misinterpreted what you want to do with this, but I feel like maybe this could be interesting stuff to keep in mind either way.

Initial coarse conversion of damage types to decls.

Removing adjust/set/get loss procs.

Coarse take_damage/apply_damage/etc proc unification.

Converting older apply_damage() code to new damage handlers.

Replacing species mod getters with damage modifier list/getter.

Disambiguating various damage procs.

Blob damage now uses a shared take_damage() proc on /atom.

Structures now use shared take_damage() proc.

Walls now use shared take_damage proc.

Shields now use shared take_damage proc.

Organs now use shared damage procs.

Mech components now use shared damage proc.

Items and machines use shared damage proc.

Post-rebase compile updates.

Replacing decl paths for damage handlers with old single-word defines.
@MistakeNot4892
Copy link
Contributor Author

Splitting prep work from this PR out into #4064.

@MistakeNot4892
Copy link
Contributor Author

Abandoning this for now, can revisit down the track when stuff is more unified.

@MistakeNot4892 MistakeNot4892 added the legitimate salvage This PR was closed without merge, but is desirable. label Jun 9, 2024
@PsyCommando
Copy link
Contributor

Abandoning this for now, can revisit down the track when stuff is more unified.

To clarify, you told me you wanted me to wait on this before working on my damage stuff. So, what's the plan? Should, I just go ahead with unifying damage stuff to /obj and possibly /atom level, or should I put mine off indefinitely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legitimate salvage This PR was closed without merge, but is desirable. work in progress This PR is under development and shouldn't be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants