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

Botany Rework Part 1: Mutations #31163

Merged
merged 22 commits into from
Sep 15, 2024

Conversation

drakewill-CRL
Copy link
Contributor

@drakewill-CRL drakewill-CRL commented Aug 18, 2024

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

  • I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

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.

@deltanedas
Copy link
Contributor

#25507

@drakewill-CRL
Copy link
Contributor Author

#25507

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.

Copy link
Contributor

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

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Aug 18, 2024
@github-actions github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Aug 18, 2024
@drakewill-CRL drakewill-CRL marked this pull request as ready for review August 19, 2024 03:09
@github-actions github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Aug 19, 2024
Copy link
Contributor

github-actions bot commented Sep 2, 2024

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

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Sep 2, 2024
@github-actions github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Sep 9, 2024
@Emisse
Copy link
Contributor

Emisse commented Sep 9, 2024

this one needs notafet approval

Copy link
Contributor

@Partmedia Partmedia left a 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:

  1. 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.

  2. 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 of EntityEffect. So each of the currently implemented EntityEffects 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.

Content.Server/Botany/Systems/BotanySystem.Produce.cs Outdated Show resolved Hide resolved
Content.Server/Botany/Systems/BotanySystem.Seed.cs Outdated Show resolved Hide resolved
Content.Server/Botany/Systems/MutationSystem.cs Outdated Show resolved Hide resolved
Content.Server/EntityEffects/Effects/PlantSpeciesChange.cs Outdated Show resolved Hide resolved

protected override string? ReagentEffectGuidebookText(IPrototypeManager prototype, IEntitySystemManager entSys)
{
return "TODO";
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Resources/Prototypes/Hydroponics/randomMutations.yml Outdated Show resolved Hide resolved
Content.Shared/Random/RandomPlantMutation.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Partmedia Partmedia left a 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.

Content.Server/Botany/Systems/MutationSystem.cs Outdated Show resolved Hide resolved

protected override string? ReagentEffectGuidebookText(IPrototypeManager prototype, IEntitySystemManager entSys)
{
throw new NotImplementedException();
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Content.Server/Botany/Systems/MutationSystem.cs Outdated Show resolved Hide resolved
Content.Server/Botany/Systems/MutationSystem.cs Outdated Show resolved Hide resolved
Comment on lines 43 to 54
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);
}
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Content.Server/EntityEffects/Effects/PlantMutateKudzu.cs Outdated Show resolved Hide resolved
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();
Copy link
Contributor

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".

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@Partmedia Partmedia left a 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?

@drakewill-CRL
Copy link
Contributor Author

drakewill-CRL commented Sep 15, 2024

I did the math at some point to determine the odds of each mutation with 1u of mutagen. I rounded up in my notes:

  • 1 bit: .4% (consume gasses)
  • 2 bits: .7% (sentient)
  • 4 bits: 1.4% (exude gasses)
  • 5 bits: 1.8% (tolerance changes)
  • 10 bits: 3.6% (change species, kudzu, and 'fun' mutations)
  • 20 bits: 7.2% (change chemicals)
  • 30 bits: 10.9% (unviable)

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:

  • {[Key = Unviable, 14]}
  • {[Key = ChangeChemicals, 9]}
  • {[Key = Slippery, 8]}
  • {{Key = ChangeSpecies, 7]}
  • {[Key = ChangeLigneous, 7]}
  • {[Key = ChangeTurnIntoKudzu, 6]}
  • {[Key = ChangeHarvest, 6]}
  • {[Key = ChangeLifespan, 4]}
  • {[Key = ChangeProduction, 4]}
  • {[Key = ChangeYield, 4]}
  • {[Key = ChangeScreaming, 4]}
  • {[Key = ChangeWeedTolerance, 3]}
  • {[Key = ChangeSeedless, 3]}
  • {[Key = ChangeHighPressureTolerance, 3]}
  • {[Key = ChangeNutrientConsumption, 3]}
  • {[Key = ChangeEndurance, 3]}
  • {[Key = ChangeLowPressureTolerance, 2]}
  • {[Key = ChangePestTolerance, 2]}
  • {[Key = ChangeHeatTolerance, 2]}
  • {[Key = ChangeExudeGasses, 2]}
  • {[Key = ChangePotency, 2]}
  • {[Key = Sentient, 1]}
  • {[Key = Bioluminescent, 1]}
  • {[Key = ChangeConsumeGasses, 1]}
  • {[Key = ChangeWaterConsumption, 1]}
  • {[Key = ChangeMaturation, 1]}
  • {[Key = ChangeToxinsTolerance, 1]}

Random is random, but that lines up with expectations fairly well for a small sample size.

@Partmedia Partmedia merged commit 1dec19c into space-wizards:master Sep 15, 2024
11 checks passed
@Partmedia
Copy link
Contributor

Thanks for your work and testing on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Botany Status: Needs Review This PR requires new reviews before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants