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

refactor: Moves simplemob atmos and temperature requirements to DCS elements. #25619

Merged

Conversation

warriorstar-orion
Copy link
Contributor

@warriorstar-orion warriorstar-orion commented May 24, 2024

What Does This PR Do

This PR moves code for atmospherics and temperature changes for simplemobs to elements. As a refresher, elements are like components except every element is a singleton that the objects it's attached to shares. Bespoke elements allow for variance in initial arguments, making singletons for every unique set of arguments.

Why It's Good For The Game

This is part of the piecemeal port of TG's basic mobs. It decouples the logic of homeostasis from the basic mob implementation itself, making the mob implementation smaller. In our case, since we don't have basic mobs yet, I've used it to backwards refactor simplemobs as well. This allows us to make sure the code works before the rest of the port.

Images of changes

Examples of the same element being used by multiple mobs for each element type

2024_05_24__17_35_28__

Testing

In progress.

  • Verified in VV that mobs are being grouped by element per atmos/breathing requirements
  • Tested spiders like Araneus took atmos damage from being in a pure plasma environment
  • Tested corgis die in space
  • Tested space bats don't die in space
  • Test slimes and cold weakness / fire affinity

Changelog

NPFC

@ParadiseSS13-Bot ParadiseSS13-Bot added Testmerge Requested This PR has a pending testmerge request -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels May 24, 2024
@SteelSlayer SteelSlayer added the Refactor This PR will clean up the code but have the same ingame outcome label May 25, 2024
@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting review This PR is awaiting review from the review team and removed -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally labels May 25, 2024
@warriorstar-orion warriorstar-orion marked this pull request as ready for review May 25, 2024 03:23
@BiancaWilkson
Copy link
Contributor

Slimes and their cold weakness and fire affinity should probably be tested. I can see that possibly causing some weirdness.

@warriorstar-orion
Copy link
Contributor Author

Slimes and their cold weakness and fire affinity should probably be tested. I can see that possibly causing some weirdness.

Looks good:

2024_06_01__07_23_42__Paradise Station 13
2024_06_01__07_26_06__Paradise Station 13

Copy link
Contributor

@lewcc lewcc left a comment

Choose a reason for hiding this comment

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

Little nits, code generally looks good.

code/datums/elements/atmos_requirements.dm Outdated Show resolved Hide resolved
code/datums/elements/body_temperature.dm Outdated Show resolved Hide resolved
code/datums/elements/body_temperature.dm Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Jun 9, 2024
Copy link
Contributor

@Sirryan2002 Sirryan2002 left a comment

Choose a reason for hiding this comment

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

just add the comments lewcc wanted

Co-authored-by: Luc <[email protected]>
Signed-off-by: warriorstar-orion <[email protected]>
@warriorstar-orion
Copy link
Contributor Author

just add the comments lewcc wanted

Ahh this conflicts with a bit of the MILLA changes. I'll look into it.

@ParadiseSS13-Bot ParadiseSS13-Bot added -Status: Awaiting merge This PR is ready for merge and removed -Status: Awaiting review This PR is awaiting review from the review team labels Jun 18, 2024
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Jun 20, 2024
@Sirryan2002 Sirryan2002 requested a review from lewcc June 24, 2024 04:44
Copy link
Contributor

@lewcc lewcc left a comment

Choose a reason for hiding this comment

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

LGTM, pending TM

@ParadiseSS13-Bot ParadiseSS13-Bot added the Testmerge Active This PR is currently testmerged on production label Jul 11, 2024
@SteelSlayer
Copy link
Member

This was TM'd for a round and this was runtime was happening quite a lot. Pulling the TM for now.

[2024-07-11T04:25:57] Runtime in code/controllers/subsystem/processing/SSdcs.dm:38: list index out of bounds
   proc name: GetIdFromArguments (/datum/controller/subsystem/processing/dcs/proc/GetIdFromArguments)
   src: Datum Component System (/datum/controller/subsystem/processing/dcs)
   call stack:
   Datum Component System (/datum/controller/subsystem/processing/dcs): GetIdFromArguments
   Datum Component System (/datum/controller/subsystem/processing/dcs): GetElement
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): _RemoveElement
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): remove_atmos_requirements
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): Destroy
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): Destroy
   qdel(Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged), 0)
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): death
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): death
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): update_stat
   ...
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): adjustBruteLoss
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): apply_damage
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): bullet_act
   Pirate Gunner (/mob/living/simple_animal/hostile/pirate/ranged): bullet_act
   the kinetic force (/obj/item/projectile/kinetic): Bump
   the kinetic force (/obj/item/projectile/kinetic): Move
   the kinetic force (/obj/item/projectile/kinetic): process
   Projectiles (/datum/controller/subsystem/processing/projectiles): ignite
   Master (/datum/controller/master): Loop
   Master (/datum/controller/master): StartProcessing
[2024-07-11T05:00:06] Runtime in code/controllers/subsystem/processing/SSdcs.dm:38: list index out of bounds
   proc name: GetIdFromArguments (/datum/controller/subsystem/processing/dcs/proc/GetIdFromArguments)
   usr: Lawliet (CKEY) (/mob/living/carbon/human)
   usr.loc: The floor (167,87,2) (/turf/simulated/floor/plasteel)
   src: Datum Component System (/datum/controller/subsystem/processing/dcs)
   call stack:
   Datum Component System (/datum/controller/subsystem/processing/dcs): GetIdFromArguments
   Datum Component System (/datum/controller/subsystem/processing/dcs): GetElement
   the dark purple baby slime (35... (/mob/living/simple_animal/slime): _RemoveElement
   the dark purple baby slime (35... (/mob/living/simple_animal/slime): remove_atmos_requirements
   the dark purple baby slime (35... (/mob/living/simple_animal/slime): Destroy
   the dark purple baby slime (35... (/mob/living/simple_animal/slime): Destroy
   qdel(the dark purple baby slime (35... (/mob/living/simple_animal/slime), 0)
   /datum/food_processor_process/... (/datum/food_processor_process/mob/slime): process_food
   /datum/food_processor_process/... (/datum/food_processor_process/mob/slime): process_food
   /datum/food_processor_process/... (/datum/food_processor_process/mob/slime): process_food
   Slime Processor (/obj/machinery/processor): attack_hand
   Lawliet (/mob/living/carbon/human): UnarmedAttack
   Lawliet (/mob/living/carbon/human): ClickOn
   Slime Processor (/obj/machinery/processor): MouseDrop

@ParadiseSS13-Bot ParadiseSS13-Bot removed the Testmerge Active This PR is currently testmerged on production label Jul 11, 2024
@S34NW S34NW added the Stale This PR has been left inactive and requires an update. label Jul 30, 2024
@S34NW
Copy link
Member

S34NW commented Jul 30, 2024

This PR has been marked as stale due to lack of activity, please continue working on the PR and/or address any comments within 7 days or we will have to close it.

@warriorstar-orion
Copy link
Contributor Author

This was broken by #25972 which snuck in before it, I've been waiting for @chuga-git to pin down the issue. I'll do a merge master in the meantime.

@warriorstar-orion warriorstar-orion changed the title refactor: elementize simplemob homeostasis refactor: Moves simplemob atmos and temperature requirements to DCS elements. Aug 3, 2024
@S34NW S34NW removed the Stale This PR has been left inactive and requires an update. label Aug 4, 2024
@S34NW
Copy link
Member

S34NW commented Aug 6, 2024

#26387 should fix runtimes here

@ParadiseSS13-Bot ParadiseSS13-Bot added the Testmerge Active This PR is currently testmerged on production label Aug 7, 2024
@Danchi299
Copy link
Contributor

class components?
in BYOND programming?
that is surely interesting

Copy link
Member

@Burzah Burzah left a comment

Choose a reason for hiding this comment

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

This has been on TM for awhile now after it was adjusted, with no reported issues. Looks good to me.

code/modules/mob/living/simple_animal/simple_animal.dm Outdated Show resolved Hide resolved
@Burzah Burzah added this pull request to the merge queue Aug 15, 2024
Merged via the queue into ParadiseSS13:master with commit 90e52f7 Aug 15, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting merge This PR is ready for merge Refactor This PR will clean up the code but have the same ingame outcome Testmerge Active This PR is currently testmerged on production Testmerge Requested This PR has a pending testmerge request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants