-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
Add simple energy balance info without dictionaries #5780
base: master
Are you sure you want to change the base?
Add simple energy balance info without dictionaries #5780
Conversation
The lead programmer for Thrive is currently on vacation until 2025-01-07. Until then other programmers will try to make pull request reviews, but please be patient if your PR is not getting reviewed. PRs may be merged after multiple programmers have approved the changes (especially making sure to ensure style guide conformance and gameplay testing are good). If there are no active experienced programmers who can perform merges, PRs may need to wait until the lead programmer is back to be merged. |
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 already a very good start, but I kind of want the architecture split to go a bit deeper even if that means a bit of code duplication (but on balance removes the virtual methods).
{ | ||
return cached; | ||
} | ||
|
||
var maximumMovementDirection = MicrobeInternalCalculations.MaximumSpeedDirection(species.Organelles); | ||
|
||
cached = new EnergyBalanceInfoSimple(); |
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.
Could you add a TODO comment here like // TODO: check if caching instances of these objects would be better than always recreating
? I expect there to be no big difference but for future reference I'd like a comment here pointing out this possibility.
} | ||
|
||
/// <summary> | ||
/// Computes the energy balance for the given organelles in biome | ||
/// </summary> | ||
/// <param name="result"> | ||
/// The energy balance info that is returned. This may be an EnergyBalanceSimple or an EnergyBalanceFull. |
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.
<see cref=""/>
should be used when referring to code in the XML to make it navigable and Jetbrains will check to make sure the reference is valid.
@@ -227,15 +232,11 @@ public static EnergyBalanceInfo ComputeEnergyBalance(IReadOnlyList<OrganelleTemp | |||
/// If true, then the required input compounds to run at the given energy balance are stored per energy producer | |||
/// </param> | |||
/// <param name="cache">Auto-Evo Cache for speeding up the function</param> | |||
public static EnergyBalanceInfo ComputeEnergyBalance(IReadOnlyList<OrganelleTemplate> organelles, | |||
public static void ComputeEnergyBalance(EnergyBalanceInfoSimple result, IReadOnlyList<OrganelleTemplate> organelles, |
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 method needs to be split into two even if that results in code duplication as I think it will be much cleaner to remove the dummy methods from the lite version of energy balance that do absolutely nothing, like SetupTrackingForRequiredCompounds
. That's not optimal but I think it might be warranted in this very core piece of code that needs maximum performance. We just need to add big enough warning comments about the duplicated code that needs to all be changed if one copy is changed.
using System.Collections.Generic; | ||
|
||
/// <summary> | ||
/// Info regarding a microbe's energy balance in a patch |
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 summary comments should be made distinct between this and the simple variant.
{ | ||
} | ||
|
||
public virtual void SetupTrackingForRequiredCompounds() |
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 quite suspicious of this kind of "trap" in the code where a method does absolutely nothing when used on an object in a specific state even though the name of the method is very clearly implying it should do something.
Brief Description of What This PR Does
Create an
EnergyBalanceInfoSimple
class that doesn't record the consumption and production. This is used for the auto evo since it doesn't require the consumption and production information.Also add an
EnergyBalanceInfoFull
class that inherits fromEnergyBalanceInfoSimple
but with the dictionaries for consumption and production. This is used for the editor display.Since there are virtual methods now, this will likely be detrimental to performance. However I wasn't able to test the significance of this compared to removing the dictionary allocations for auto-evo since there doesn't see to be a great way to benchmark auto-evo.
Related Issues
Closes #5426
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.