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

Interaction Chain & Damage refactoring #9838

Closed
wants to merge 39 commits into from

Conversation

PowerfulBacon
Copy link
Member

@PowerfulBacon PowerfulBacon commented Sep 16, 2023

About The Pull Request

This completely refactors how damage is applied internally as well as some parts of the interaction chain. The goal of this PR is to make it so that in the future damage is easier to refactor and expand upon and the interaction procs have more sensible names and are more intuative to use (attackbk, AttackBy, attack, attack_hand, attack_obj, etc. are a bit of a mess and its hard to tell which one you should use and when).

I am making this PR now so that I am forced to finish it and also so that I can record a todo list.

This PR has been designed around making the following things easy to implement in a way that is cohesive and doesn't feel like it is band-aided on top of a messy foundation (I won't necessarilly implement these things, but they are options for the future):

  • Wounds/Injuries/Other complex damage modes
  • Combat Mode
  • Swing Combat

Damage Sources + Effects

How do we handle the problem of many different damage sources having many different effects which apply differently across many different types? (Sharp applies bleeding which causes limbs to bleed but does nothing to machines).
In order to achieve this we go through a middle layer of effects. Sharp can call a bleeding proc on /atom, which is then overriden on the types that want special behaviours. This means that no damage_source has any specific behaviours per atom, but instead call generic functions which can be overriden. This means that we can add new effects, apply affects to many different damage sources and have those effects apply to many different damage targets without making a complete mess of type checking.

image

Interaction Chain

A few procs in the interaction chain have been slightly refactored to make them more intuative to use. We have completely seperated out the section of interaction that handles attack from the section that handles interactions with items.
Most importantly, this means that almost all uses of attackby() are now a use of item_interact instead. Attackby will be removed upon completion, so any ports from TG won't compile there rather than having it compile but with different behaviour. The only change is that item_interact now returns TRUE when the interaction was successful and blocks the parent action. Technically we could make it return a special value when calling the parent function to emulate old behaviour instead, that would probably clean this PR up quite a bit and make it so that attackby() is a simple rename instead.

image

Attacking/Damage Cleanup

Almost everything to do with damage application has been rolled into the generic damage procs and damage_source. If you want to apply bleeding to a target, you now damage them with a damage source that applies bleeding. Armour calculation is handled by damage_source too, so you don't need to find the right damage proc when you want to respect armour penetration.

Damage sources can be applied in 2 ways:

  • apply_direct: Directly applies the damage to the target from a non-physical source (chemical applications, no attacker/item used to attack)
  • deal_attack: Causes a mob/item to attack the target, playing the attack animation and damage sound.

Damage_source now will deal with:

  • Armour penetration
  • Attack animation
  • Attack text
  • Bleeding
  • Dismemberment
  • Damage source transformation (Armour might change a sharp attack to a blunt attack for example)

Damage types allow you to specify different damage types and what procs they call to be applied. For example, burn damage calls adjustFireLoss when called on a target.

image

I also renamed adjustBruteLoss to adjustBruteLossAbstract to force it to be checked when porting and because I don't want people to directly adjust brute loss but instead go through the damage source system.

TODO List

  • Handle negative damage and healing
  • Convert any remaining attack_by functions into item_interact or on_attacked.
  • Move attack messages into the damage functions
  • Run through the adjustXLoss functions and move them to the new system.
  • /obj/item/proc/attack_mob_target(mob/living/M, mob/living/user) contains animation calls which should be standardised
  • /obj/item/proc/attack_mob_target(mob/living/M, mob/living/user) contains log_combat calls which should be standardised
  • /obj/item/proc/attack_mob_target(mob/living/M, mob/living/user) Surgery steps need to be handled in item_interact instead of attack calls
  • We need handling for hitting people on help intent not dealing damage
  • /obj/item/tk_grab/afterattack needs checking, we play an animation in there but do we need to?
  • Do we actually need attack_hulk still, or can we make this a generic thing?
  • Damage taking sounds should be inside the damage source system instead of on items/mobs.
  • Handle target_zone inside of /datum/damage_source/proc/apply_direct
  • Airlock crush code makes mobs scream. This should be moved out to the respective mobs (aliens should roar upon taking damage etc.)
  • Attack message and attack sound needs to be in the attack chain
  • Move dealing damage out of attack_slime and instead use that as an override for the default deal_damage behaviour that slimes/xenos/animals have.
  • /mob/living/simple_animal/hostile/syndicate/factory/boss/updatehealth()
  • updatehealth() should only deal with UI updates and player facing things, subtracting from health is so damn cheap that we should do it when adjusting the damage losses
  • deal_generic_attack needs to take into account melee_damage_type
  • deal_generic_attack for slimes (rand(20) damage if child, rand(30) if adult, *1.1 if transformeffects & SLIME_EFFECT_RED)
Proc Remaining to check
attackby 190
adjustBruteLoss 324 0
adjustOxyLoss 149
adjustToxLoss 209
adjustFireLoss 194
adjustCloneLoss 50 0
setCloneLoss 12
adjustStaminaLoss 150
setStaminaLoss 9 This is fine
apply_damage_old 15
take_bodypart_damage 38
take_overall_damage 36

Why It's Good For The Game

This should make any significant refactors to damage such as more advanced wounds, complex damage types and swing combat a lot easier.
This also makes item interaction things a lot more intuative for coders creating new interactions.

The biggest downside is that this will completely destroy compatability with TG for damage/health code meaning that if we wanted swing combat or wounds it would be incredibly difficult to port from TG and will have to be written using this system instead. In theory wounds should be very easy to add with generic damage sources and the biggest amount of code changes that would need to be done would be with how you heal wounds instead. We could leave legacy support for attackby in the code and some old signals there so that things can be ported without being updated, but then over time that could lead to code being added which uses an old system.

The other downside is that this conflicts with legitimately everything.

Testing Photographs and Procedure

image
image

Changelog

🆑
refactor: Refactors damage application.
refactor: Refactors the item interaction chain slightly.
refactor: Refactors updatehealth() proc to be called automatically at the end of the stack frame.
/:cl:

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the Tools label Sep 16, 2023
@PowerfulBacon
Copy link
Member Author

PowerfulBacon commented Sep 16, 2023

We can abuse the behaviour of spawn(0) to make it so that we only update health at the end of the stack frame and don't have to manually pass in the var to update health.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to toggle this back when done!

@PowerfulBacon
Copy link
Member Author

Since this PR is far too big to properly review, will probably go with bulking out unit tests and then having testmerges to ensure that its stable and ready. Unit tests will prevent future regressions too

@PowerfulBacon PowerfulBacon marked this pull request as ready for review December 21, 2023 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants