-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Botany Rework Part 1: Mutations #31163
Botany Rework Part 1: Mutations #31163
Conversation
I saw that, and it's very likely I will be using some/most of that logic in future PRs. I am very glad that there's something to build off of for the "make plants their own entity" step of the rework when I get there. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
this one needs notafet approval |
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.
Thanks for your work on this PR. I generally agree with the approach taken here, except for two issues:
-
That numeric mutations are a simple random number instead of the thermometer code system that existed before. You claim that the old system "obfuscate[d] the actual changes of something happening behind 3 layers of math", but the math is actually quite simple and has these desirable properties:
- The "tiers" of the mutations are quantized, e.g. for a fixed number of bits you can get potency 0.2, 0.4, 0.6, 0.8, 1... instead of 0.3123847895
- Each successful mutation moves the number from where it was higher or lower, i.e. you don't get random jumps (you started at 0 but suddenly go to 0.8, and then it goes back down to 0.2)
All of which the new system would throw away in the name of apparent "simplicity". In reality it just does less.
-
The implementation of the YAML
RandomPlantMutationList
. It now seems that you can define mutations in YAML, but you can't because you have to create new mutations by defining a new subclass ofEntityEffect
. So each of the currently implementedEntityEffect
s duplicate the randomization code (plantholder.Seed.Foo = random.NextFloat(MinValue, MaxValue)
). Further, anyone looking to add new mutations is likely going to blindly copy/paste this boilerplate code. Encouraging copy/paste of boilerplate is something we'd like to avoid because it sort of defeats the entire purpose of moving to YAML. I do like that you no longer have to manually sum the bits, but I would like to point out that you could have summed the bits at compile time with a source generator.
Overall I do like the use of EntityEffects
instead of putting the plant-altering code in one class.
I've also pointed out a number of style nitpicks inline.
|
||
protected override string? ReagentEffectGuidebookText(IPrototypeManager prototype, IEntitySystemManager entSys) | ||
{ | ||
return "TODO"; |
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.
Instead of all of these returning TODO, why not leave the default implementation inherited from EntityEffect
?
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.
EntityEffect's version is abstract, which requires an implementation here. PlantAdjustAttribute provides that implementation for all the 'change a plant stat number' effects.
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 think this is much better with PlantChangeStat
. Thanks for taking the time to fix the nits. It's going to take a bit more time for me to review the changes in depth and do some testing, but I'm mostly happy with the code structure.
|
||
protected override string? ReagentEffectGuidebookText(IPrototypeManager prototype, IEntitySystemManager entSys) | ||
{ | ||
throw new NotImplementedException(); |
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.
It would be best to actually implement these. But you can wait to implement this after we make sure everything else is okay.
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.
This string is probably here because EntityEffects were originally ReagentEffects, and all the reagents and their effects are supposed to show up in the guidebook. I'm not sure if we even want mutations to show up in the guidebook.
if (member.FieldType == typeof(float)) | ||
{ | ||
var floatVal = (float)currentValObj; | ||
mutationSys.MutateFloat(ref floatVal, MinValue, MaxValue, Steps); | ||
member.SetValue(plantHolder.Seed, floatVal); | ||
} | ||
else if (member.FieldType == typeof(int)) | ||
{ | ||
var intVal = (int)currentValObj; | ||
mutationSys.MutateInt(ref intVal, (int)MinValue, (int)MaxValue, Steps); | ||
member.SetValue(plantHolder.Seed, intVal); | ||
} |
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.
This no longer scales with the amount of mutagen your pour into the plant holder, no?
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.
Mutagen never affected the results of the mutation, only the probability of it occurring. In the existing MutateFloat/Int code, mult is only used on the initial check with probBitFlip, not when determining valMutated.
Severity is checked in this PR in CheckRandomMutations, on line 30, and is multiplied with the base odds to the same result that it was when it was multiplied against (bits / totalbits) in the existing code.
CrossBool(ref result.TurnIntoKudzu, a.TurnIntoKudzu); | ||
CrossBool(ref result.CanScream, a.CanScream); | ||
|
||
CrossGasses(ref result.ExudeGasses, a.ExudeGasses); | ||
CrossGasses(ref result.ConsumeGasses, a.ConsumeGasses); | ||
|
||
result.BioluminescentColor = Random(0.5f) ? a.BioluminescentColor : result.BioluminescentColor; | ||
result.Mutations = result.Mutations.Where(m => Random(0.5f)).Union(a.Mutations.Where(m => Random(0.5f))).DistinctBy(m => m.Name).ToList(); |
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'm not super familiar with LINQ and so it could be helpful to leave a comment for passers-by as to what the intent of this LINQ statement is.
In particular, since you've already selected from result.Mutations
with 50% chance, why do you need to select 50% randomly from a.Mutations
, instead of just unioning with a.Mutations
?
The cross logic used to be simply "take A 50% of the time, and if not A then B".
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 can add the comment clarifying it. I'll do a long explanation here for you and figure out a way to shorten it in the file if I can.
Since Mutations is now a list of entity effects, this takes each entry on the first plant (result.Mutations) 50% of the time, then takes each entry on the 2nd plant (a.Mutations) 50% of the time, merges both of those together in the Union call, and Distinct keeps only entries with a unique name (discarding the second one in case both list had it and both rolled that 50% to add it).
I think this is the most performant way to put this line together. Alternates would be making a new result list, putting each thing from both plants Mutations list on there 50% of the time if the results list doesn't already have an entry with the same name, and making that results.Mutations.
I could also do the Union first, and then pick all the present entries in the combined list 50% of the time and Distinct to confirm there's no duplicates, but that does more copying/deleting memory work because the list is bigger to start and removes things from it.
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.
Thanks for the summary. If I understand the change in behavior correctly:
Old Behavior
For each type of mutation, 50% chance to take from plant A, otherwise take from plant B.
New Behavior
For each type of mutation, 50% chance to take from plant A, otherwise there's a 50% of the remaining chance to take from plant B (so overall 25% chance), and then a 25% chance of losing a mutation from crossing.
Is my understanding correct, and if so, was this the intent?
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.
For all of the stats on SeedData, Cross() works the way it did before. Note that the mutations that apply changes directly to stats and parameters on SeedData are set to fire and change the value, then not persist. This list is for special effects that occur to the plant and/or its produce (currently Bioluminescent, Slippery, and Sentient. This PR enables the potential for new effects to be added without checking for each one each time, and I have roughly a dozen in mind for future plans in my Hydro rework doc).
The 2 checks for the 2 lists are independent.
Control Example: P1 and P2 have no effect mutation: The resulting cross will have no effect mutations.
EX 1: P1 has Bioluminescent, P2 does not. There is a 50% chance the resulting cross has Bioluminescent.
EX 2: P1 has Bioluminescent and Slippery, P2 has Slippery. There is a 50% chance the resulting cross has Bioluminescent, and a 75% it has Slippery. (1/4 both checks are false, 1/4 both checks are true and Distinct removes the duplicate, 2/4 one is true and the other is false). This matches up with typical Mendelian genetics if we pretend for now that all plants have 1 dominant and 1 recessive gene involved in these checks.
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'm satisfied with the rework. Thanks for your work in putting this together and helping out in review.
In testing this in-game, I felt that the mutation rate felt faster than it was before, and in particular kudzu felt like it was more common than I remember. I don't have a good way to quantify this numerically, but would you be willing to double check this before we go ahead and merge?
I did the math at some point to determine the odds of each mutation with 1u of mutagen. I rounded up in my notes:
Confirming kudzu was and is on the 10-bit value, so it should happen about as often as the species changing according to the code. I set up some simple logging code and ran 150u of mutagen through a plant 1u at a time, expecting to get about 103 or so mutations. I got 104, the results for rolls are as follows:
Random is random, but that lines up with expectations fairly well for a small sample size. |
Thanks for your work and testing on this! |
About the PR
This PR changes how mutations are handled in the botany system.
In addition, the math behind applying mutations has been simplified. Instead of the fake-gene-bit setup that was present, each one now has a percentage chance of being applied equal to having a bit flip. The secondary math for mapping value changes to gene-bits has also been thrown out in favor of simple random rolls. This averages out the same in the long run and shouldn't affect gameplay since there's no gameplay besides 'apply mutagen and wait' for this.
Why / Balance
Botany's a stagnating system. The code is a mess, so lots of new ideas get put off until its reworked. It hasn't been reworked in 2+ years of people saying there's a rework coming because it's a big mess. People are less interested in the job because there's nothing new there in years, which reduces interest in fixing the mess. This chips away at the mess as part of my botany rework design doc.
This mostly does not affect game balance, because it's a refactor. Some bugs will be fixed in the process and its possible that players will be able to feel a difference in Botany if the percentage odds were not converted correctly. Balancing that will simply be tweaking numbers in YAML. The biggest balance change in the randomness rework is that produce will be able to hit the high and low values for stats much more frequently than they do now, because the code no longer shoves all the mutation ranges towards the middle.
Technical details
Instead of each mutation being a flag that gets checked at some unique point in BotanySystem somewhere, they're now EntityEffects that get applied when the mutation occurs and when produce is harvested. One new list was added to SeedData so that multiple other fields could be removed.
All the non-stat-change mutations that have been rolled are added to the Mutations list, and get applied to the plant when the mutation occurs or when a seed with the mutation is planted. Produce get mutations applied at harvest if they apply to the produce, and carry all of the plant's mutations over as a seed. This gets rid of the one-off checks for things like Slippery, Bioluminescent, Sentient, etc.
The base odds of a mutation applying should be equal to the odds of the original mutation check. It pretended to have 1 bit flip (on averge) per mutation power, and odds of each mutation was the odds of one of its bit being flipped (1 /275 * bits). The 'thermometer code' applied for numbers will be replaced with simple random rolls, as both average out to the middle value. The new checks are much easier to understand and don't obfuscate the actual changes of something happening behind 3 layers of math. The biggest player-facing change is that Potency will be able to get over 65 significantly more often than it did in the previous system, but it will be just as common to get low values as high ones.
Mutation definitions have been moved to a .yml file. These include the odds per tick per mutagen strength of that mutation applying that tick, the effect applied, if it applies to the plant and/or its produce. This makes mutations simpler to add and edit.
This PR is limited specifically to the mutation logic. Improving other aspects of the system will be done in other PRs per the design document. Mutations was chosen first because its got the largest amount of one-off checks scattered all over that could be consolidated. Once this is merged, mutations could be contributed to the codebase with minimal extra work for later botany refactor PRs.
Media
I don't think there's any good videos/pictures for a backend refactor.
Requirements
Breaking changes
MutationSystem.MutateSeed() is dramatically different in function and has a changed signature. The rest of Botany stuff shouldn't be impacted
Changelog
No player-facing changes.