-
-
Notifications
You must be signed in to change notification settings - Fork 497
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
Implement new auto-evo prototype algorithm #1335
Implement new auto-evo prototype algorithm #1335
Conversation
@@ -18,7 +18,7 @@ public static void Simulate(SimulationConfiguration parameters) | |||
|
|||
while (parameters.StepsLeft > 0) | |||
{ | |||
RunSimulationStep(parameters, speciesToSimulate, random); |
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 like to keep the random as a parameter as I can imagine many scenarios where the simulation step needs randomness even if it isn't currently needed,
add _ = random
to stop it from being detected as unused.
bool lowSpecies = species.Count <= Constants.AUTO_EVO_LOW_SPECIES_THRESHOLD; | ||
bool highSpecies = species.Count >= Constants.AUTO_EVO_HIGH_SPECIES_THRESHOLD; |
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.
Shouldn't this be removed from here? Now that there is kinda an auto evo algorithm this hack to keep the species count reasonable shouldn't be needed.
foreach (var currentSpecies in species) | ||
var speciesEnergies = new Dictionary<MicrobeSpecies, float>(species.Count); | ||
|
||
var totalPhotosynthesisScore = 0f; |
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.
Either use 0.0f
or float totalPhotosynthesisScore
here instead of 0f
var photosynthesisScore = 0f; | ||
foreach (var organelle in species.Organelles) | ||
{ | ||
if (organelle.Definition.InternalName == "chloroplast") |
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 is not extensible, instead check if the organelle has a photosynthesis process, even that should probably be checked by finding a process with sunlight as the only input and making glucose.
var predationScore = 0f; | ||
foreach (var organelle in species.Organelles) | ||
{ | ||
if (organelle.Definition.InternalName == "pilus") |
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.
Should check for having the pilus component in the organelle instead of by name.
var chemosynthesisScore = 0f; | ||
foreach (var organelle in species.Organelles) | ||
{ | ||
if (organelle.Definition.InternalName == "chemoplast") |
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.
Same thing here as the photosynthesis check.
There are now some changes on master regarding how the compound data is defined in the biomes, so you'll want to merge master to this PR and fix things. |
int currentPopulation = populations.GetPopulationInPatch(currentSpecies, patch); | ||
int populationChange = random.Next( | ||
-Constants.AUTO_EVO_RANDOM_POPULATION_CHANGE, Constants.AUTO_EVO_RANDOM_POPULATION_CHANGE + 1); | ||
var processesDoneByOrganelle = organelle.Definition.RunnableProcesses; |
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 local is not needed, right? The foreach loop could just directly use organelle.Definition.RunnableProcesses
var sunlight = SimulationParameters.Instance.GetCompound("sunlight"); | ||
var glucose = SimulationParameters.Instance.GetCompound("glucose"); |
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.
Constantly looking these up should be avoided, making these private static readonly
fields should maybe work?
I think those get initialized on the first read.
|
||
foreach (var organelle in species.Organelles) | ||
{ | ||
var processesDoneByOrganelle = organelle.Definition.RunnableProcesses; |
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 local could also be inlined.
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.
Code-wise this is good, but this simulation has some pretty big problems in terms of gameplay.
First of, in freebuild mode basically all the random species go extinct immediately.
And the ones that don't go extinct get huge amounts of population:
The second problem is that the split off species from the player when starting a game basically always go extinct in a single editor cycle.
So there are two choices:
You either make changes to fix the gameplay side for this new population simulation (or get someone to fix it).
Or you make a duplicate SimulatePatchStep
method that has the changed version in it, but it isn't used anywhere. So we would have the logic in thrive compatible format for when someone wants to go through the needed steps to fix this up.
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.
Despite trying really hard to get someone to review this as well, I couldn't get anyone to do that.
So I'll comment more. Now it's definitely better that the new species don't instantly go extinct, but the glucose energy calculation should be made more accurate. Also iron should be added as an energy source, otherwise species that use iron won't appear.
Also the energy needed for population needs to be increased by about 10 as otherwise the population numbers get way too high, which will break the spawn system.
I hope you understand me asking a bunch of changes constantly as this PR will have huge gameplay implications.
currentSpeciesEnergy += glucoseInPatch | ||
* 1 / species.Count; |
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 that the glucose score should be done similarly to the other scores by calculating how much glucose the species can turn into ATP, doesn't make sense if a species that relies on iron gets a ton of glucose score.
Hey, sorry I don't think I am a good person to review intricate gameplay changes as I haven't been following the project long enough. |
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.
One thing I noticed is that after you play for long enough, the number of species grows large and that will probably cause a performance hit especially when the player respawns since lots of different species microbes will be spawned at once. I'm not sure, perhaps it's just the spawn system needing a rework, or maybe #1361 will fix this.
One thing I've noticed about the new algorithm is that there's no real way for species to go extinct naturally; if a species is capturing any energy at all from the environment, it's going to get a strictly positive 1/X share of the total. So a high species penalty is maybe still needed to keep species counts reasonable. |
Removing flat 200 population causes discontinuous mass extinctions still (e.g. patch has capacity for 2000 population, 10 species mean average species has 200 population, so when the high species penalty fires they all go extinct), so I'll need to do more tweaking to keep species numbers in a reasonable range. |
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 have just that one comment about the code. Otherwise it looks good.
@buckly90 and/or @Kasterisk could you do some game balance testing with this?
src/microbe_stage/Patch.cs
Outdated
@@ -111,6 +111,23 @@ public int GetSpeciesPopulation(Species species) | |||
return SpeciesInPatch[species]; | |||
} | |||
|
|||
public float GetPatchChunkTotalCompoundAmount(Compound compound) |
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.
Rename this to GetTotalChunkCompoundAmount
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 didn't reread this whole thing, but I suppose now that my last code related comment was fixed, all that's left is get this approved from game balance perspective @buckly90 ?
From what Tjwhale has told me and what I got from testing, it seems this implementation mostly works as intended so that's good!
That being said, I think Holomanga did a nice job with incorporating this into Thrive, these issues are more with the prototype than what he did I think. |
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'll mark this as needing changes so that this doesn't get merged before Buckly's comments are addressed.
A more thought out update to my earlier comment: |
@@ -107,7 +113,11 @@ private static void RunSimulationStep(SimulationConfiguration parameters, List<S | |||
{ | |||
// Simulate the species in each patch taking into account the already computed populations | |||
SimulatePatchStep(parameters.Results, entry.Value, | |||
<<<<<<< HEAD |
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 looks like a partly done merge artifact.
@Holomanga Hi. I had a look at your code and you've made a load of nice progress with this system, which is really cool. The prototype was quite incomplete which I think explains quite a lot of the odd behaviour. I wanted to ask if you have any interest in joining the team and working on it with me? I made a roadmap here for how to proceed with it which I think will work. https://forum.revolutionarygamesstudio.com/t/a-roadmap-for-implementing-auto-evo/703?u=tjwhale And I'm around to give help and theoretical support (though unfortunately not much programming stuff) to help get it done. I totally understand if you're busy or not interested and it's a major part of the project so it would mean a lot to us if we could get it done. |
This commit finishes the rebasing, so it's in a state where I can start working on it again. Over discord, IceDjuro proposed to have predation subtract energy from a species' pool of energy - this allows energies to go to zero, which would reduce a big problem (energy and hence populations are always positive so extinctions never occur). |
… to compensate for the lost energy.
The philosopy behind this change is that before, predation was magic extra energy coming into the system, but now the new energy is removed later which means that it's just a redistribution of energy in favour of predators. |
…ants and added checks to make sure that sent population amount is reasonable
the energy amounts a bit
now it just prints out the exception message and quits the task thread
I added extinction by a minimum population, and did some general tweaks on this. |
Implements the auto-evo algorithm linked in #1326