-
Notifications
You must be signed in to change notification settings - Fork 523
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
Cryogenics: restrict cryo pods to cryo meds, restore limited metabolism #1533
Cryogenics: restrict cryo pods to cryo meds, restore limited metabolism #1533
Conversation
// Frontier: keep a list of cryogenics reagents. The pod will only filter these out from the provided solution. | ||
private static readonly string[] CryogenicsReagents = ["Cryoxadone", "Aloxadone", "Doxarubixadone", "Opporozidone", "Necrosol", "Traumoxadone", "Stelloxadone"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like this: frankly, it should be searchable by MetabolismGroup without LINQ.
Whenever the reagents get loaded from YAML, you could look through the effects, associate them with the metabolism groups they have effects for, and then access that set in some lifecycle call here to build this array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would recommend not using a hard coded list. it will make it makes it harder to maintain given somebody might add, re-name, remove, update, something in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that (as far as I know) there isn't a good, existing way to look it up. I can tell you what this should be - every reagent that can be metabolized as a Cryogenic drug.
Association's done from the individual reagents, and by the effects they have (can't think of an example of association here that isn't 1:1, but it's possibly 1:N off the top of my head). This isn't being stored at all in the MetabolismGroup, so we can't have a nice look up at runtime.
cryo does not take away from those, the changes to the "potency" do not increase how much or how fast it heals it just reduced how much chem was used. the med bed is still very powerful as it cleans up damage across the board without any meds. the benefit to the future tech of cryo was that it saved on chems and allowed for safer use of cryo chems.
what if rather than limiting the amount of things that can be metabolized, we limited how much can be metabolized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working as intended
I like having my human blood stream suck ass with regents. 😢
} | ||
else | ||
{ | ||
splitSolution.AddReagent(currentReagent.Reagent, toTakePer); | ||
RemoveReagent(currentReagent.Reagent, toTakePer); | ||
Contents.RemoveSwap(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I had mentioned in the private messages, it is bad practice to remove from an iterator while iterating threw it.
the argument of "but everywhere else does it" falls in line with "if they jump off a bridge would you"
quality can not be improved if it is ignored for consistently
that's not directed at you, but anybody and everybody who works on code
as developers it is our responsibility to protect quality where we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but we are also tied with upstream. If you want to move away from the code they have, alright, that's fine, but it must be done with intent. That is not the case here. I would believe that it is easier to maintain a set of solution splitting functions that look and act similarly than four and an odd duck.
// Frontier: keep a list of cryogenics reagents. The pod will only filter these out from the provided solution. | ||
private static readonly string[] CryogenicsReagents = ["Cryoxadone", "Aloxadone", "Doxarubixadone", "Opporozidone", "Necrosol", "Traumoxadone", "Stelloxadone"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would recommend not using a hard coded list. it will make it makes it harder to maintain given somebody might add, re-name, remove, update, something in the future.
After looking into Upstream metabolism refactor (space-wizards/space-station-14#19383) we should stay away from making any metabolism code changed till it pass, if this PR revert most of them to upstream code this is actually good.
|
humans can metabolize .01 unit per tick. take that you smelly humans 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised. Fair points on the functions themselves.
// Frontier: keep a list of cryogenics reagents. The pod will only filter these out from the provided solution. | ||
private static readonly string[] CryogenicsReagents = ["Cryoxadone", "Aloxadone", "Doxarubixadone", "Opporozidone", "Necrosol", "Traumoxadone", "Stelloxadone"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that (as far as I know) there isn't a good, existing way to look it up. I can tell you what this should be - every reagent that can be metabolized as a Cryogenic drug.
Association's done from the individual reagents, and by the effects they have (can't think of an example of association here that isn't 1:1, but it's possibly 1:N off the top of my head). This isn't being stored at all in the MetabolismGroup, so we can't have a nice look up at runtime.
} | ||
else | ||
{ | ||
splitSolution.AddReagent(currentReagent.Reagent, toTakePer); | ||
RemoveReagent(currentReagent.Reagent, toTakePer); | ||
Contents.RemoveSwap(i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but we are also tied with upstream. If you want to move away from the code they have, alright, that's fine, but it must be done with intent. That is not the case here. I would believe that it is easier to maintain a set of solution splitting functions that look and act similarly than four and an odd duck.
About the PR
This PR addresses side effects of the Cryo Overhaul PR, and reverts most changes in behaviour to metabolism.
I have added versions of per-reagent SplitSolution functions (one with whitelist, one without) that are more consistent with other methods in the class.
Why / Balance
The change in behaviour from metabolizing a fixed number of things to an unlimited number of things affects the metabolism of food, of medicine, and of poisons. It removed most differences between species apart from arachnids metabolizing less frequently.
The goal is to keep cryo strong, but appropriately strong, and to avoid overreliance or abuse of cryo pods for non-cryo meds. I believe injections, pills, topical meds and bedrest should ideally all have a place in medical gameplay.
How to test
Media
Testing metabolism: non-cryo meds are randomly metabolized if too many in solution, vs. consistently metabolized cryo meds
Testing cryo pod extraction: only cryo meds are extracted, non-cryo reagents are left in the beaker
Breaking changes
Content.Shared.Chemistry.Components.Solution
:SplitSolutionEvenly
has been renamed toSplitSolutionPerReagent
, its parameters have not changed.SplitSolutionPerReagentWithOnly
.Changelog
🆑