Add dots indicating player population on patch map #5880

CI09 · 2025-02-06T19:33:17Z

Brief Description of What This PR Does

Might need some visual stuff and other tweaks but it theoretically can be merged.

Related Issues

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.

GameDungeon · 2025-02-06T19:51:20Z

An image would be nice, so we can give graphics feedback.

CI09 · 2025-02-06T20:36:26Z

hhyyrylainen · 2025-02-06T20:40:06Z

I think the dots should be more desaturated to look like more as just background stuff. That would help with them not messing with the readability of the connection lines.

hhyyrylainen · 2025-02-07T12:36:34Z

src/microbe_stage/editor/PatchMapPopulationIndicator.cs

+/// <summary>
+///   Dots scattered around patch map nodes, indication population of player species
+/// </summary>
+public partial class PatchMapPopulationIndicator : Control


There's going to be a ton of these, so I think that actually this shouldn't be a node type with a script attached. And instead just I'd inline this code to the patch map drawer class (though it should be cleanly put into a separate section as it is already a pretty long file). I think the performance impact will be much less when there's just simple graphics used for the indicators rather than nodes with scripts attached.

hhyyrylainen · 2025-02-07T12:37:26Z

src/microbe_stage/editor/PatchMapDrawer.cs

+    /// <summary>
+    ///   Player species ID for player population indicator (dots on patch map)
+    /// </summary>
+    public uint PlayerSpeciesID { get; set; }


I think this could default to -1 (or uint.MaxValue if C# complains) so that this doesn't accidentally match anything if there's a bug elsewhere in the code that causes a species with ID = 0 to be created.

hhyyrylainen · 2025-02-07T12:39:33Z

src/microbe_stage/editor/PatchMapDrawer.cs

+
+            for (var i = 0; i < playerPopulation * 0.001; ++i)
+            {
+                var indicator = populationIndicatorScene.Instantiate<PatchMapPopulationIndicator>();


I think it would be worthwhile probably to have a cache of these objects as I think otherwise this will increase the amount of memory allocations quite a lot here.

Overall the architecture isn't far off from being pretty much exactly what I would have done for this feature. So if this is slightly tweaked and the visuals improved a bit, this PR will be good to go.

By cache you mean a list created in this create-patch-node method, a list in the patch map drawer class, or something else completely? I'm not questioning why, since you are much more experienced but I'm curious how it reduces allocations

A Queue or a Stack in the patch map drawer class, so it can just create them once and then reuse them.

Once the queue is filled up once with unused objects, the next time the dots need to be put in the map, they can be pulled from the queue and refilled. So this way the dots are instantiated just once, instead of being instantiated on each map draw.

I started to work on this but I realized that indicators are children of patch nodes, which are deleted.

What if they weren't? What if they were added as children to the patch map itself? AFAIK the patch nodes are added as children to the drawer node in order to position them and make them display at the right position on screen.

CI09 · 2025-02-07T14:54:08Z

Also I get this, I don't know what it is:

hhyyrylainen · 2025-02-07T15:03:22Z

Seems like pretty normal Godot shutdown errors to me. There's probably an open bug report to godot about those. At least I remember looking up the rendering server error and seeing there was an open bug with it. I think I'm watching it on github and I don't remember seeing an email about it being closed, so the issue is still probably open.

CI09 · 2025-02-07T17:54:30Z

On below image is volcanic vents patch and only adjacent patch inhabited by player:

All 3 patched inhabited by player. Uppermost has about 6k population, rest 2-3k:

I don't know if the caching was done the right way though.

Edit: Oh and dots are behind the panel. I wanted to give other patch map elements ZIndex (greater than zero) but it would be on top of pause menu. So I put indicators underneath all else then.

hhyyrylainen · 2025-02-07T17:58:27Z

Edit: Oh and dots are behind the panel. I wanted to give other patch map elements ZIndex (greater than zero) but it would be on top of pause menu. So I put indicators underneath all else then.

You can reorder the node that contains the dot in relation to its siblings to control what is drawn on top of what without messing with zindex or draw behind parent properties.

CI09 · 2025-02-07T18:13:08Z

Oh I just reminded myself that you said tutorial will be needed. It can be done in a seperate PR, or in this. I will be doing else for today though so I won't implement tutorial today.

hhyyrylainen · 2025-02-07T19:20:22Z

As long as the effect is made simple enough, I think we can do without having a tutorial. Well as long as there's some thriveopedia page or something that explains the feature.

…nary-Games/Thrive.git into player_population_dots

hhyyrylainen

I think this is in better shape than when I last looked at this. There's still some minor stuff in the code I'd like to see improved.

hhyyrylainen · 2025-02-10T09:04:59Z

src/microbe_stage/editor/PatchMapDrawer.cs

@@ -11,6 +11,8 @@ public partial class PatchMapDrawer : Control
    [Export]
    public bool DrawDefaultMapIfEmpty;

+    public uint PlayerSpeciesID = int.MaxValue;


Non-godot properties shouldn't be mixed in so this field should be after LineContainerPath

hhyyrylainen · 2025-02-10T09:06:17Z

src/microbe_stage/editor/PatchMapDrawer.cs

@@ -149,6 +157,8 @@ public override void _Ready()
        lineContainer = GetNode<Control>(LineContainerPath);

        nodeScene = GD.Load<PackedScene>("res://src/microbe_stage/editor/PatchMapNode.tscn");
+        populationIndicatorScene = GD.Load<PackedScene>(


This is just a single node so for performance I don't think this should be made as a separate scene, but just instantiate the nodes with the right texture (load the texture here) in this class.

hhyyrylainen · 2025-02-10T09:06:44Z

src/microbe_stage/editor/PatchMapDrawer.cs

@@ -1047,6 +1057,13 @@ private void DrawPatchLinks()
    /// </summary>
    private void RebuildMap()
    {
+        playerSpeciesPopulationIndicators = new List<Control>();


The list should be stored persistently as otherwise this allocates memory unnecessarily each time the map is drawn.

hhyyrylainen · 2025-02-10T09:07:22Z

src/microbe_stage/editor/PatchMapDrawer.cs

@@ -1047,6 +1057,13 @@ private void DrawPatchLinks()
    /// </summary>
    private void RebuildMap()
    {
+        playerSpeciesPopulationIndicators = new List<Control>();
+        foreach (var node in populationIndicatorContainer.GetChildren())


GetChildren is still here, this needs to go I think GetChildren is a sign of an architecture problem. Use a persistent list for knowing all the existing indicators that way you can also avoid the type cast on line 1063.

hhyyrylainen · 2025-02-10T09:08:20Z

src/microbe_stage/editor/PatchMapDrawer.cs

+        var playerSpecies = patch.FindSpeciesByID(PlayerSpeciesID);
+        if (playerSpecies != null)
+        {
+            var playerPopulationIndicatorAmount = (int)Math.Ceiling(


As this whole long block is inside this if, I'd recommend making this as a separate helper method.

hhyyrylainen · 2025-02-10T09:09:05Z

src/microbe_stage/editor/PatchMapDrawer.cs

+
+            for (int i = 0; i < indicatorExcess; ++i)
+            {
+                playerSpeciesPopulationIndicators.Last().QueueFree();


I think it would be better to just hide the excess indicators, I don't expect us to have so many excess ones that they'd cause an issue during the lifetime of the map drawer node.

hhyyrylainen · 2025-02-10T09:09:29Z

src/microbe_stage/editor/PatchMapDrawer.cs

+        if (playerSpecies != null)
+        {
+            var playerPopulationIndicatorAmount = (int)Math.Ceiling(
+                patch.GetSpeciesSimulationPopulation(playerSpecies) * 0.004);


Making the constant here into a value in Constants would make it easier for someone else to tweak how many circles appear.

hhyyrylainen · 2025-02-10T09:10:39Z

src/microbe_stage/editor/PatchMapDrawer.cs

+
+                if (noCached)
+                {
+                    populationIndicatorContainer.AddChild(indicator);


This could be combined with the above if where this is instantiated for easier reading of this block of code.

TheESN · 2025-02-10T13:53:11Z

Personally I am not really fan of this idea with dots, since it might make patch maps more busy looking in my opinion

hhyyrylainen · 2025-02-10T13:54:44Z

That's why I asked it to be made lower contrast so that it just looks like a bit of background flair and not really anything the player needs to pay attention to.

CI09 · 2025-02-10T15:58:26Z

The idea was to persuade player to use migration tool more. It's safer for player to spread onto as many patches as possible but migration can be easily forgotten in my opinion as main feature of editor is, well, cell editor. If it gets into RC, we can ask what others think of the visual look and maybe come up with something better.

…c stuff)

hhyyrylainen

I playtested this now and noticed a bug where on the initial generation (normal and freeplay) the population indicators do not display at all. I think that needs to be fixed. Also using the "move to this patch" button causes an error immediately upon press (I commented the callstack separately). So there's a few gameplay bugs and some quite small remaining code related problems.

hhyyrylainen · 2025-02-11T08:18:32Z

simulation_parameters/Constants.cs

@@ -1705,6 +1705,9 @@ public static class Constants

    public const float PATCH_GENERATION_CHANCE_BANANA_BIOME = 0.03f;

+    // Constants for displaying the patch map


This comment doesn't seem to match what this is on? This should be an xml summary comment along the lines of "Sets how many population indicator dots appear on the patch map"

hhyyrylainen · 2025-02-11T08:39:31Z

simulation_parameters/Constants.cs

@@ -1705,6 +1705,9 @@ public static class Constants

    public const float PATCH_GENERATION_CHANCE_BANANA_BIOME = 0.03f;

+    // Constants for displaying the patch map
+    public const float PLAYER_POPULATION_POPULATION_FOR_PER_INDICATOR = 1.0f / 300.0f;


Shouldn't the name of this constant be the inverse? Because this is how many indicators there are per population. That's the difference between population / population per indicator and population * indicators per population. So I think this should be renamed for clarity.

hhyyrylainen · 2025-02-11T08:47:40Z

src/microbe_stage/editor/PatchMapDrawer.cs

+            {
+                indicator = new TextureRect
+                {
+                    Texture = GD.Load<Texture2D>("res://assets/textures/gui/bevel/MapDotIndicator.svg"),


This texture should be loaded just once in _Ready and not tried to be reloaded for each indicator.

hhyyrylainen · 2025-02-11T09:00:09Z

src/microbe_stage/editor/PatchMapDrawer.cs

+            playerSpeciesPopulationIndicators.Count);
+
+        // Hide excess from the end of the list
+        for (int i = 0; i < indicatorExcess; ++i)


If you want to keep things this way, I'll accept it, but I think a slightly easier way to program this would be to use a variable like, nextIndicatorIndex that starts at 0 and goes up for each used indicator. Then the excess indicators can be hidden by looping from the ending nextIndicatorIndex value up to the indicators count. And the noCached variable can be calculated as nextIndicatorIndex >= playerSpeciesPopulationIndicators.Count.

Now with the persistent index for used indicators, I think this hiding excess indicators code needs to be put into a single place that is called after the entire map is drawn. Otherwise the last indicators get hidden over and over (and I think your indicatorExcess math was not updated anyway so that would lead to a bug in any case).

hhyyrylainen · 2025-02-11T09:01:21Z

src/microbe_stage/editor/PatchMapDrawer.cs

+                indicator.Show();
+            }
+
+            indicator.MouseFilter = MouseFilterEnum.Ignore;


This should be in the indicator creation as it doesn't seem like this value is ever updated so it wastes time to reapply the value on each loop iteration.

hhyyrylainen · 2025-02-11T09:01:38Z

src/microbe_stage/editor/PatchMapDrawer.cs

+            var nodeModifier = node.Position.LengthSquared();
+            var modifierSinus = MathF.Sin(i);
+
+            playerSpeciesPopulationIndicators.Add(indicator);


Isn't this a major bug where the same indicator objects are added again and again to the same list?

I think this is probably what caused this error when I moved patches in the editor:

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection. (Parameter 'index') at System.Collections.Generic.List`1.get_Item(Int32 index) at PatchMapDrawer.AddPlayerPopulationIndicators(Patch patch, Species playerSpecies, Control node, Vector2 position) in /home/hhyyrylainen/Projects/Thrive/src/microbe_stage/editor/PatchMapDrawer.cs:line 1141 at PatchMapDrawer.AddPatchNode(Patch patch, Vector2 position) in /home/hhyyrylainen/Projects/Thrive/src/microbe_stage/editor/PatchMapDrawer.cs:line 1122 at PatchMapDrawer.RebuildMap() in /home/hhyyrylainen/Projects/Thrive/src/microbe_stage/editor/PatchMapDrawer.cs:line 1069 at PatchMapDrawer._Process(Double delta) in /home/hhyyrylainen/Projects/Thrive/src/microbe_stage/editor/PatchMapDrawer.cs:line 175 at Godot.Node.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/Node.cs:line 2395 at Godot.CanvasItem.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/CanvasItem.cs:line 1505 at Godot.Control.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Generated/GodotObjects/Control.cs:line 2912 at PatchMapDrawer.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in /home/hhyyrylainen/Projects/Thrive/.godot/mono/temp/obj/Debug/Godot.SourceGenerators/Godot.SourceGenerators.ScriptMethodsGenerator/PatchMapDrawer_ScriptMethods.generated.cs:line 318 at Godot.Bridge.CSharpInstanceBridge.Call(IntPtr godotObjectGCHandle, godot_string_name* method, godot_variant** args, Int32 argCount, godot_variant_call_error* refCallError, godot_variant* ret) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/CSharpInstanceBridge.cs:line 24

Good you caught that.

hhyyrylainen · 2025-02-11T09:08:13Z

assets/textures/gui/bevel/MapDotIndicator.svg

I think that maybe if the dot was fully filled in and made slightly less in opacity, it would be a lot more subtle effect? That way I think there wouldn't be really any complaining about the added visuals.

hhyyrylainen · 2025-02-11T10:00:32Z

src/microbe_stage/editor/PatchMapDrawer.cs

+        // Hide excess from the end of the list
+        for (int i = 0; i < indicatorExcess; ++i)
+        {
+            playerSpeciesPopulationIndicators[playerSpeciesPopulationIndicators.Count - i].Hide();


Also when I'm looking at this code, isn't the math wrong? Because playerSpeciesPopulationIndicators.Count is not valid index, the first valid index is playerSpeciesPopulationIndicators.Count - 1 but that doesn't happen as i starts off as 0.

Also I think I just realized also another problem: the player being in multiple patches will reset the indicators. There needs to be a persistent nextIndicatorIndex variable that is only cleared on a full map re-draw not during a map draw when this method can be called multiple times which means that the separate calls for different patches on the map will interfere with each other.

CI09 · 2025-02-11T14:19:54Z

CI09 · 2025-02-12T19:38:42Z

Excess indicators are now hidden after all patch nodes are dones.
Though I get this error:

hhyyrylainen · 2025-02-12T20:25:48Z

Isn't that again just the usual Godot texture errors on game exit that happen very often? I think it's unlikely any of your changes in this PR would affect that.

hhyyrylainen · 2025-02-18T09:03:28Z

This probably won't make it in for 0.8.1 so I'll push this back to release 0.8.2.

revolutionary-bot · 2025-02-18T09:03:33Z

We are currently in feature freeze until the next release.
If your PR is not just a simple fix, then it may take until the release to get reviewed and merged.

Add dots indication player population

cb4ceec

CI09 added the review label Feb 6, 2025

CI09 added this to the Release 0.8.1 milestone Feb 6, 2025

CI09 requested review from a team February 6, 2025 19:33

Style fix

608718f

hhyyrylainen reviewed Feb 7, 2025

View reviewed changes

hhyyrylainen requested changes Feb 7, 2025

View reviewed changes

Caching, visuals and performance tweaks

4268a2c

CI09 added 4 commits February 7, 2025 18:59

Style fix

5d8212c

Reorder children

86c2910

Remove ZIndex change

1e9ddfa

Fix wrong parenting

4efda91

CI09 added 7 commits February 7, 2025 20:24

Style fix

797aea9

Style fix

e67f36a

Merge branch 'master' into player_population_dots

03603a3

Style fix?

fa9f846

Merge branch 'player_population_dots' of https://github.com/Revolutio…

20de572

…nary-Games/Thrive.git into player_population_dots

Style fix

d55eb81

Style fix

63ccc78

hhyyrylainen requested changes Feb 10, 2025

View reviewed changes

CI09 added 3 commits February 10, 2025 19:10

Address requested changes (removed seperate scene for indicator, logi…

194cc1b

…c stuff)

Style fix

98a1bec

Style fix

c168bc5

hhyyrylainen requested changes Feb 11, 2025

View reviewed changes

hhyyrylainen reviewed Feb 11, 2025

View reviewed changes

CI09 added 2 commits February 11, 2025 15:04

Refactor

d1752e5

Change indicator dot image a bit

a8fd79c

CI09 added 2 commits February 11, 2025 15:23

Move dots cluster further from patch node

80a7e8c

Fixes

330293e

Style fix

fcf8f69

hhyyrylainen modified the milestones: Release 0.8.1, Release 0.8.2 Feb 18, 2025

		@@ -1705,6 +1705,9 @@ public static class Constants

		public const float PATCH_GENERATION_CHANCE_BANANA_BIOME = 0.03f;

		// Constants for displaying the patch map

Add dots indicating player population on patch map #5880

Are you sure you want to change the base?

Add dots indicating player population on patch map #5880

Conversation

CI09 commented Feb 6, 2025 • edited Loading

GameDungeon commented Feb 6, 2025

CI09 commented Feb 6, 2025

hhyyrylainen commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CI09 commented Feb 7, 2025

hhyyrylainen commented Feb 7, 2025

CI09 commented Feb 7, 2025 • edited Loading

hhyyrylainen commented Feb 7, 2025

CI09 commented Feb 7, 2025 • edited Loading

hhyyrylainen commented Feb 7, 2025

hhyyrylainen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheESN commented Feb 10, 2025

hhyyrylainen commented Feb 10, 2025

CI09 commented Feb 10, 2025

hhyyrylainen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CI09 commented Feb 11, 2025

CI09 commented Feb 12, 2025

hhyyrylainen commented Feb 12, 2025

hhyyrylainen commented Feb 18, 2025

revolutionary-bot commented Feb 18, 2025

CI09 commented Feb 6, 2025 •

edited

Loading

CI09 commented Feb 7, 2025 •

edited

Loading

CI09 commented Feb 7, 2025 •

edited

Loading