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

Fix processes' toggle buttons not matching their actual state #5632

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/general/world_effects/PhotosynthesisProductionEffect.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ private void ApplyCompoundsAddition()
float oxygenBalance = 0;
float co2Balance = 0;

Dictionary<BioProcess, float> tempStorage = new();

foreach (var species in patch.SpeciesInPatch)
{
// Only microbial photosynthesis and respiration are taken into account
Expand All @@ -67,7 +69,7 @@ private void ApplyCompoundsAddition()
if (balance.TotalConsumption < balance.TotalProduction)
balanceModifier = balance.TotalConsumption / balance.TotalProduction;

ProcessSystem.ComputeActiveProcessList(microbeSpecies.Organelles, ref microbeProcesses);
ProcessSystem.ComputeActiveProcessList(microbeSpecies.Organelles, ref microbeProcesses, tempStorage);

foreach (var process in microbeProcesses)
{
Expand Down
23 changes: 14 additions & 9 deletions src/gui_common/ChemicalEquation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ public partial class ChemicalEquation : VBoxContainer
/// </summary>
private bool hasNoInputs;

private bool lastToggle = true;

[Signal]
public delegate void ToggleProcessPressedEventHandler(ChemicalEquation thisEquation);

Expand Down Expand Up @@ -102,14 +100,22 @@ public bool ShowToggle

public bool ProcessEnabled
{
get => lastToggle;
get
{
if (EquationFromProcess == null)
return true;

return EquationFromProcess.Enabled;
}
set
{
if (value == lastToggle)
if (EquationFromProcess == null)
{
GD.PrintErr("Display info isn't assigned to a ChemicalEquation, can't toggle process");
return;
dligr marked this conversation as resolved.
Show resolved Hide resolved
}

lastToggle = value;
ApplyProcessToggleValue();
EmitSignal(SignalName.ToggleProcessPressed, this, value);
}
}

Expand Down Expand Up @@ -149,7 +155,6 @@ public bool ShowFullSecondText
public override void _Ready()
{
UpdateEquation();
ApplyProcessToggleValue();
}

public override void _EnterTree()
Expand Down Expand Up @@ -245,6 +250,8 @@ private void UpdateEquation()

// Environment conditions
UpdateEnvironmentPart(environmentalInputs);

ApplyProcessToggleValue();
}

private void UpdateHeader()
Expand Down Expand Up @@ -376,8 +383,6 @@ private string GetEnvironmentLabelText()
private void ToggleButtonPressed(bool toggled)
{
ProcessEnabled = toggled;

EmitSignal(SignalName.ToggleProcessPressed, this);
}

private void ApplyProcessToggleValue()
Expand Down
2 changes: 2 additions & 0 deletions src/microbe_stage/IProcessDisplayInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ public interface IProcessDisplayInfo : IEquatable<IProcessDisplayInfo>
/// </summary>
public float CurrentSpeed { get; }

public bool Enabled { get; }

/// <summary>
/// The limiting compounds in speed. Or null if not set
/// </summary>
Expand Down
27 changes: 22 additions & 5 deletions src/microbe_stage/MicrobeHUD.cs
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ private void OnTranslationsChanged()
UpdateColonySizeForMacroscopic();
}

private void ToggleProcessPressed(ChemicalEquation equation)
private void ToggleProcessPressed(ChemicalEquation equation, bool enabled)
{
if (!stage!.HasAlivePlayer || !stage.Player.Has<BioProcesses>())
return;
Expand All @@ -852,9 +852,26 @@ private void ToggleProcessPressed(ChemicalEquation equation)
return;
}

ref var processes = ref stage.Player.Get<BioProcesses>();
if (stage.Player.Has<MicrobeColony>())
{
ref var colony = ref stage.Player.Get<MicrobeColony>();

foreach (var colonyMember in colony.ColonyMembers)
{
ref var bioProcesses = ref colonyMember.Get<BioProcesses>();
ToggleProcessOnEntity(equation, enabled, ref bioProcesses);
}
}
else
{
ref var bioProcesses = ref stage.Player.Get<BioProcesses>();
ToggleProcessOnEntity(equation, enabled, ref bioProcesses);
}
}

var activeProcesses = processes.ActiveProcesses;
private void ToggleProcessOnEntity(ChemicalEquation equation, bool enabled, ref BioProcesses bioProcesses)
{
var activeProcesses = bioProcesses.ActiveProcesses;

if (activeProcesses == null)
return;
Expand All @@ -864,10 +881,10 @@ private void ToggleProcessPressed(ChemicalEquation equation)
for (int i = 0; i < processesCount; ++i)
{
// Update speed of the process controlled by the GUI control that signaled this change
if (equation.EquationFromProcess.MatchesUnderlyingProcess(activeProcesses[i].Process))
if (equation.EquationFromProcess!.MatchesUnderlyingProcess(activeProcesses[i].Process))
{
var process = activeProcesses[i];
process.SpeedMultiplier = equation.ProcessEnabled ? 1 : 0;
process.SpeedMultiplier = enabled ? 1 : 0;
activeProcesses[i] = process;
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/microbe_stage/MicrobeStage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,7 @@ public override void OnReturnFromEditor()

var workData1 = new List<Hex>();
var workData2 = new List<Hex>();
var tempMemory = new Dictionary<BioProcess, float>();

var playerSpecies = Player.Get<SpeciesMember>().Species;

Expand All @@ -650,13 +651,13 @@ public override void OnReturnFromEditor()
earlySpeciesType.MulticellularCellType = earlySpeciesType.Species.Cells[0].CellType;

cellProperties.ReApplyCellTypeProperties(Player, earlySpeciesType.MulticellularCellType,
earlySpeciesType.Species, WorldSimulation, workData1, workData2);
earlySpeciesType.Species, WorldSimulation, workData1, workData2, tempMemory);
}
else
{
ref var species = ref Player.Get<MicrobeSpeciesMember>();
cellProperties.ReApplyCellTypeProperties(Player, species.Species, species.Species, WorldSimulation,
workData1, workData2);
workData1, workData2, tempMemory);
}

var playerPosition = Player.Get<WorldPosition>().Position;
Expand Down
4 changes: 3 additions & 1 deletion src/microbe_stage/MicrobeVisualOnlySimulation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public sealed class MicrobeVisualOnlySimulation : WorldSimulation

private readonly List<Hex> hexWorkData1 = new();
private readonly List<Hex> hexWorkData2 = new();
private readonly Dictionary<BioProcess, float> tempProcessListMemory = new();

// Base systems
private AnimationControlSystem animationControlSystem = null!;
Expand Down Expand Up @@ -143,7 +144,8 @@ public void ApplyNewVisualisationMicrobeSpecies(Entity microbe, MicrobeSpecies s

// Do a full update apply with the general code method
ref var cellProperties = ref microbe.Get<CellProperties>();
cellProperties.ReApplyCellTypeProperties(microbe, species, species, this, hexWorkData1, hexWorkData2);
cellProperties.ReApplyCellTypeProperties(microbe, species, species, this, hexWorkData1, hexWorkData2,
tempProcessListMemory);

// TODO: update species member component if species changed?
}
Expand Down
6 changes: 2 additions & 4 deletions src/microbe_stage/ProcessList.cs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,6 @@ private ChemicalEquation CreateEquation(StrictProcessDisplayInfoEquality process

equation.Connect(SignalName.ToggleProcessPressed, new Callable(this, nameof(HandleToggleProcess)));

equation.ProcessEnabled = process.DisplayInfo.CurrentSpeed > 0;

if (ProcessesTitleColour != null)
equation.DefaultTitleFont = ProcessesTitleColour;

Expand All @@ -99,8 +97,8 @@ private ChemicalEquation CreateEquation(StrictProcessDisplayInfoEquality process
return equation;
}

private void HandleToggleProcess(ChemicalEquation equation)
private void HandleToggleProcess(ChemicalEquation equation, bool enabled)
{
EmitSignal(SignalName.ToggleProcessPressed, equation);
EmitSignal(SignalName.ToggleProcessPressed, equation, enabled);
}
}
4 changes: 2 additions & 2 deletions src/microbe_stage/ProcessPanel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}

private void ToggleProcessToggled(ChemicalEquation equation)
private void ToggleProcessToggled(ChemicalEquation equation, bool enabled)
{
EmitSignal(SignalName.ToggleProcessPressed, equation);
EmitSignal(SignalName.ToggleProcessPressed, equation, enabled);
}
}
2 changes: 2 additions & 0 deletions src/microbe_stage/ProcessSpeedInformation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public ProcessSpeedInformation(BioProcess process)

public float CurrentSpeed { get; set; }

public bool Enabled => true;

/// <summary>
/// Efficiency is a measure of how well the environment is favorable to the process.
/// </summary>
Expand Down
34 changes: 17 additions & 17 deletions src/microbe_stage/ProcessStatistics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,19 @@ public class ProcessStatistics
/// a ThreadLocal in <see cref="Systems.ProcessSystem"/> as that was causing the game process to lock up in
/// Godot 4 (for unknown reasons). See: https://github.com/Revolutionary-Games/Thrive/issues/4989 for context.
/// </summary>
private List<BioProcess>? temporaryRemovedItems;
private List<TweakedProcess>? temporaryRemovedItems;

/// <summary>
/// The processes and their associated speed statistics
/// </summary>
/// <remarks>
/// <para>
/// This uses <see cref="BioProcess"/> rather than <see cref="TweakedProcess"/> as the key so that equality
Copy link
Member

Choose a reason for hiding this comment

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

This is another point that I think is causing some pretty major architectural problems here. I didn't yet check every single place, that these changes impact, but my impression is that all of the code depending on the assumptions about the code that has been changed in this PR have not been updated yet.

/// comparison matches based on the process type not the process type and speed. All processables should
/// combine their processes to run correctly with speed tracking.
/// </para>
/// <para>
/// This is JSON ignore to ensure that this object can exist in saves, but won't store non-savable information
/// like the process statistics object. That's the situation now but maybe some other design would be better...
/// </para>
/// </remarks>
[JsonIgnore]
public Dictionary<BioProcess, SingleProcessStatistics> Processes { get; } = new();
public Dictionary<TweakedProcess, SingleProcessStatistics> Processes { get; } = new();

public void MarkAllUnused()
{
Expand All @@ -49,7 +44,7 @@ public void RemoveUnused()
{
lock (Processes)
{
temporaryRemovedItems ??= new List<BioProcess>();
temporaryRemovedItems ??= new List<TweakedProcess>();

foreach (var entry in Processes)
{
Expand All @@ -71,13 +66,12 @@ public void RemoveUnused()
}
}

public SingleProcessStatistics GetAndMarkUsed(BioProcess forProcess)
public SingleProcessStatistics GetAndMarkUsed(TweakedProcess forProcess)
{
#if DEBUG
dligr marked this conversation as resolved.
Show resolved Hide resolved
if (forProcess == null!)
if (forProcess.Process == null!)
throw new ArgumentException("Invalid process marked used");
#endif

lock (Processes)
{
if (Processes.TryGetValue(forProcess, out var entry))
Expand Down Expand Up @@ -109,7 +103,7 @@ public class SingleProcessStatistics : IProcessDisplayInfo

private Dictionary<Compound, float>? precomputedEnvironmentInputs;

public SingleProcessStatistics(BioProcess process,
public SingleProcessStatistics(TweakedProcess process,
float keepSnapshotTime = Constants.DEFAULT_PROCESS_STATISTICS_AVERAGE_INTERVAL)
{
this.keepSnapshotTime = keepSnapshotTime;
Expand All @@ -120,11 +114,11 @@ public SingleProcessStatistics(BioProcess process,
/// <summary>
/// The process these statistics are for
/// </summary>
public BioProcess Process { get; }
public TweakedProcess Process { get; }

public bool Used { get; internal set; }

public string Name => Process.Name;
public string Name => Process.Process.Name;

public IEnumerable<KeyValuePair<Compound, float>> Inputs =>
LatestSnapshot?.Inputs.Where(p => !IProcessDisplayInfo.IsEnvironmental(p.Key)) ??
Expand All @@ -135,7 +129,7 @@ public SingleProcessStatistics(BioProcess process,
throw new InvalidOperationException("No snapshot set");

public IReadOnlyDictionary<Compound, float> FullSpeedRequiredEnvironmentalInputs =>
precomputedEnvironmentInputs ??= Process.Inputs
precomputedEnvironmentInputs ??= Process.Process.Inputs
.Where(p => IProcessDisplayInfo.IsEnvironmental(p.Key.ID))
.ToDictionary(p => p.Key.ID, p => p.Value);

Expand All @@ -154,6 +148,8 @@ public float CurrentSpeed
}
}

public bool Enabled => Process.SpeedMultiplier > 0.5f;

public IReadOnlyList<Compound>? LimitingCompounds => LatestSnapshot?.LimitingCompounds;

private SingleProcessStatisticsSnapshot? LatestSnapshot =>
Expand Down Expand Up @@ -296,7 +292,7 @@ public void Clear()

public bool MatchesUnderlyingProcess(BioProcess process)
{
return Process == process;
return Process.Process == process;
}

public bool Equals(IProcessDisplayInfo? other)
Expand Down Expand Up @@ -394,9 +390,13 @@ public AverageProcessStatistics(SingleProcessStatistics owner)
public float CurrentSpeed { get; set; }
public IReadOnlyList<Compound> LimitingCompounds => WritableLimitingCompounds;

public TweakedProcess Process => owner.Process;

public bool Enabled => owner.Process.SpeedMultiplier > 0.5f;

public bool MatchesUnderlyingProcess(BioProcess process)
{
return owner.Process == process;
return owner.Process.Process == process;
}

public bool Equals(IProcessDisplayInfo? other)
Expand Down
3 changes: 2 additions & 1 deletion src/microbe_stage/Spawners.cs
Original file line number Diff line number Diff line change
Expand Up @@ -571,9 +571,10 @@ public static float SpawnMicrobeWithoutFinalizing(IWorldSimulation worldSimulati
// just need to accept that spawning a microbe allocates a bit of temporary unnecessary memory
var workData1 = new List<Hex>();
var workData2 = new List<Hex>();
var tempMemory = new Dictionary<BioProcess, float>();

container.CreateOrganelleLayout(usedCellDefinition, workData1, workData2);
container.RecalculateOrganelleBioProcesses(ref bioProcesses);
container.RecalculateOrganelleBioProcesses(ref bioProcesses, tempMemory);

organelleCount = container.Organelles!.Count;

Expand Down
3 changes: 2 additions & 1 deletion src/microbe_stage/TweakedProcess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ public bool Equals(TweakedProcess other)
return false;

// ReSharper disable once CompareOfFloatsByEqualityOperator
return Rate == other.Rate && ReferenceEquals(Process, other.Process);
return Rate == other.Rate && SpeedMultiplier == other.SpeedMultiplier
Copy link
Member

Choose a reason for hiding this comment

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

Adding comparing the speed multiplier here, I think might break some grouping of process displays...

So I would remove this change.

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 don't think it's right, because if processes with different states are considered equal, then they aren't correctly updated on GUI, unless I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

Have you playtested and confirmed that is a bug? Kind of unhelpfully the process list detection uses equality to combine items, so too strict checking can result in a situation where duplicates get created into the process list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have play tested it without the speed multiplier check and processes' statuses didn't update.

Are you sure that too strict checking may result in duplicates? If I read the process list code right, then GUI-side processes (ChemicalEquations) that aren't equal to any of the display infos that it is given are simply removed.

Copy link
Member

Choose a reason for hiding this comment

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

It's true that if this isn't the type of equality used in the GUI combining (and that doesn't call this equality in turn), this should be fine. Though, the last time I tried to improve the process equality checks to be more accurate, I totally messed up the process speed panel to show each instance of a process separately (instead of keeping the functionality where all organelles are summed into one process of each type). I'll need to check this carefully when doing the next review (after the extra temporary memory use is solved).

&& ReferenceEquals(Process, other.Process);
}

public TweakedProcess Clone()
Expand Down
5 changes: 3 additions & 2 deletions src/microbe_stage/components/CellProperties.cs
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,10 @@ public static Vector3 CalculateExternalOrganellePosition(this ref CellProperties
/// </param>
/// <param name="workMemory1">Temporary memory used for organelle copying</param>
/// <param name="workMemory2">More temporary memory</param>
/// <param name="tempStorage">Even more temporary memory</param>
public static void ReApplyCellTypeProperties(this ref CellProperties cellProperties, in Entity entity,
ICellDefinition newDefinition, Species baseReproductionCostFrom, IWorldSimulation worldSimulation,
List<Hex> workMemory1, List<Hex> workMemory2)
List<Hex> workMemory1, List<Hex> workMemory2, Dictionary<BioProcess, float> tempStorage)
{
// Copy new cell type properties
cellProperties.MembraneType = newDefinition.MembraneType;
Expand All @@ -550,7 +551,7 @@ public static void ReApplyCellTypeProperties(this ref CellProperties cellPropert
// Reset all the duplicates organelles / reproduction progress of the entity
// This also resets multicellular creature's reproduction progress
organelleContainer.ResetOrganelleLayout(ref entity.Get<CompoundStorage>(), ref entity.Get<BioProcesses>(),
entity, newDefinition, baseReproductionCostFrom, worldSimulation, workMemory1, workMemory2);
entity, newDefinition, baseReproductionCostFrom, worldSimulation, workMemory1, workMemory2, tempStorage);

// Reset runtime colour
if (entity.Has<ColourAnimation>())
Expand Down
Loading