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

Implement a multicellular statistics panel #5820

Open
wants to merge 69 commits into
base: master
Choose a base branch
from

Conversation

dligr
Copy link
Member

@dligr dligr commented Jan 16, 2025

Brief Description of What This PR Does

Implements a multicellular statistics panel, mostly porting features from the similar panel in the cell editor.
In terms of stats, it only features speed, storage and rotatation speed as the other microbial stats don't make sense for a colony.

image

TODO:

Related Issues

Closes #3170

Progress Checklist

Note: before starting this checklist the PR should be marked as non-draft.

  • PR author has checked that this PR works as intended and doesn't
    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)
  • Initial code review passed (this and further items should not be checked by the PR author)
  • Functionality is confirmed working by another person (see above checklist link)
  • Final code review is passed and code conforms to the
    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.

@dligr
Copy link
Member Author

dligr commented Jan 16, 2025

While this PR is not complete, it would be good to hear some feedback regarding the general architecture, especially the changes made to the ProcessSystem to make the energy balance calculation work.

@hhyyrylainen hhyyrylainen added this to the Release 0.8.1 milestone Jan 17, 2025
@hhyyrylainen
Copy link
Member

especially the changes made to the ProcessSystem to make the energy balance calculation work.

I think that's fundamentally a fine change but I think it needs a bit more tweaking. I had a quick look at most of the code and I think a bigger architecture problem is that a lot of the quite complex calculation logic for the GUI display seems to be mostly duplicated from the microbe version. So finding ways to avoid the code duplication would be excellent.


private bool IsNegativeAtpProduction()
{
return energyBalanceInfo != null &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it doesn't help if there's one cell with massively positive ATP, because if there's any negative cell in the colony, that will just die when it grows in the body plan...

/// <summary>
/// Calculates the energy balance and compound balance for a colony
/// </summary>
private void CalculateEnergyAndCompoundBalance(IReadOnlyList<HexWithData<CellTemplate>> cells,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine for now, but I think it would be excellent to add a TODO here about adding some kind of selector to pick entire colony / individual cell in the future, so that the player isn't as confused about the aggregated data.

Copy link
Member

@hhyyrylainen hhyyrylainen 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 I've now reviewed all the code. So after those few bugs are solved this should be really close to done with just a few code cleanups to do. I didn't read the new OrganismStatisticsPanel too closely as I assumed it was mostly copy-pasted code (with hopefully no problems getting picked up at the same time).

(and I again got carried away with thinking about making a few minor comments before starting this review)

src/microbe_stage/OrganismStatisticsPanel.cs Outdated Show resolved Hide resolved
src/microbe_stage/OrganismStatisticsPanel.cs Outdated Show resolved Hide resolved
@hhyyrylainen
Copy link
Member

Also if it would help any (there's about 5 merge conflicts with master on this branch) I can push my local version of this branch that I rebased on master today.

@MatterNoise
Copy link

It has nothing to do with that but, will it be possible to run the cheat panel after that Pull?

@hhyyrylainen
Copy link
Member

I'm pretty sure I didn't see any code changes related to that so it will keep working as usual in the microbe editor (and still be missing from multicellular).

@hhyyrylainen
Copy link
Member

For the more complex situation of the ATP balance bar not being fully suitable for multicellular, let's just put a few TODO comments in the code. That and adding some defensive programming around the cell counts are the last things I need doing. I can help with those tomorrow if at least the merge conflicts are solved on this branch.

@dligr
Copy link
Member Author

dligr commented Jan 30, 2025

The submodule issue should be fixed now.

Regarding the ATP balance problem, wouldn't it be better to open an issue to track it, so that it can't be buried in the code?

@hhyyrylainen
Copy link
Member

hhyyrylainen commented Jan 30, 2025

I suppose that would be good so that the TODO comments can then reference also the issue URL.

Edit: meant to say that do you want to open the issue? You can if you want.

Copy link
Member

@hhyyrylainen hhyyrylainen 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 the code should be good now. I'll do a quick playtest tomorrow before merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Create an organism statistics panel for the body plan editor
3 participants