fix: Fix gas saturation, #34 #60

ImpulseCloud · 2021-03-15T03:05:36Z

If gas oversaturated, remove from busy_workers and call DistributeWorker.
If gas under-saturated and any base is mineral-oversaturated, send one of those mineral workers to that gas.

I think this can be accepted separately from Repair and AttackScout?

ImpulseCloud · 2021-03-19T03:57:24Z

May want to accept this before #61, as there's prolly conversation about what scout-worker attack should do.

alkurbatov · 2021-03-20T16:34:38Z

src/objects/GameObject.cpp

 }

-GameObject::GameObject(const sc2::Unit& unit_): m_tag(unit_.tag) {
+GameObject::GameObject(const sc2::Unit& unit_): m_unit(&unit_), m_tag(unit_.tag) {


Since we are going to keep sc2::Unit inside please pass it by pointer. Please, check the linter in advance.

alkurbatov · 2021-03-20T16:35:03Z

src/objects/GameObject.cpp

@@ -6,9 +6,10 @@
 #include "core/API.h"

 GameObject::GameObject(sc2::Tag tag_): m_tag(tag_) {
+    m_unit = gAPI->observer().GetUnit(tag_);


Do we really need this constructor now? Also, with this approach ToUnit should be removed.

alkurbatov · 2021-03-20T16:35:44Z

src/objects/GameObject.h

@@ -24,6 +24,8 @@ struct GameObject {

    static sc2::Unit ToUnit(sc2::Tag tag_);

+    const sc2::Unit* m_unit;


Style: Just const sc2::Unit* unit as it is not a member.

alkurbatov · 2021-03-20T16:36:59Z

src/plugins/Miner.cpp

@@ -44,7 +44,12 @@ void SecureVespeneIncome() {
    auto refineries = gAPI->observer().GetUnits(IsRefinery());

    for (const auto& i : refineries()) {
-        if (i->assigned_harvesters >= i->ideal_harvesters)
+        if (i->assigned_harvesters > i->ideal_harvesters) {
+            gHub->RemoveVespeneHarvester(*i);


Actually, I think the problem is in the assignment logic. How it happens that we have extra harvesters?

The SCV that builds the Refinery takes 5+ frames before it exits the 'geyser' construction area and turns around and joins the refinery. So for Terran even with perfect-working assignment, there will be 4/3. Unless we keep a separate list of vespene-workers-per-refinery.

Other than that, I think it takes ~5 frames from when the building is finished to when the "assigned_harvesters" shows a valid value. It's possible it doesn't check every frame to update "assigned_harvesters" as I have seen up to 8/3 for a geyser, which makes me think it checks every 5 frames, so if assigning the 3rd vespener on frame 1000, it will still show 2/3 (1 frame action-lag) until frame 1005, when it updates to 8/3 (including the builder).

I think, the first mitigation: we can manually assign that building scv as harvester.

The second could be making miner trigger once in 6 or more frames. So this lag could be skipped.

The third, I think is to ignore the data, provided by the game and get list of the assigned harvesters from the cache somehow.

Just running SecureVespene only every 10 frames does fix this problem completely. Fix is very simple now. :)

alkurbatov · 2021-03-21T10:06:10Z

src/plugins/Miner.cpp

@@ -140,7 +140,8 @@ void DistrubuteMineralWorker(const sc2::Unit* unit_) {

 void Miner::OnStep(Builder* builder_) {
    SecureMineralsIncome(builder_);
-    SecureVespeneIncome();
+    if (gAPI->observer().GetGameLoop() % 10 == 0)


Perhaps the whole plugin should be launched e.g. every 10th frame?
Another interesting question: how does it work with increased frames per turn value and in realtime?

Good points. I've refactored this to be part of Plugin (and changed QuarterMaster's frames_to_skip logic to match).

ImpulseCloud · 2021-03-21T19:19:21Z

Give this a look -- it's the best I can do without making a Plugin.cpp or making functions with bodies in Plugin.h

alkurbatov · 2021-03-22T07:12:10Z

There is nothing bad in adding Plugin.cpp. Plugin should stay pure virtual, the rest is not so valuable.

src/plugins/Miner.cpp

ImpulseCloud · 2021-03-28T19:30:17Z

Here's a stab at what might work. I added worker-training in Miner::OnIdle since it won't check every frame anymore (Miner::OnStep called every 7 frames), and losing 7 frames (at most) after each worker training is a little too long.

alkurbatov · 2021-03-29T17:49:27Z

src/plugins/Plugin.h

+    virtual void UpdateNextRunTurn();
+
+ protected:
+    uint32_t m_run_every_x_turns;


m_run_every_x_turns -> m_frequency.
And I assume it should be const and private. Later it is enough to check the current step by calling the module operator as you did before.

As for m_next_run_turn, I believe it is not needed. Working with frequency should be enough, cause it serves the same purpose.

You said earlier "Another interesting question: how does it work with increased frames per turn value and in realtime?" -- and that is why I had to add m_next_run_turn, because modulo will not work with "increased frames per turn value" or in RealTime, because GetGameLoop is in total frames, and not number of times that Dispatcher::OnStep has been called.

I feel that 'turn' was/is ambiguous in that I thought 'turns' meant 'frames' (GameLoops / Game Turns) but now it seems it means 'steps' (based on SetStepSize/"frames per turn value"). I think the bot variables should only think in 'frames', because otherwise changing to RealTime or changing StepSize would require many small changes to any timing values. I started using 'turns' because you were using that, but I think 'frames' should be used for precise clarity. Because of this "m_frequency" can be confusing as it changes meaning if you set RealTime/StepSize>=1, so using "m_frame_frequency" avoids this ambiguity. I think we should avoid using the word 'turn' as it is ambiguous on whether it means 'frame' or 'step'.

Actually, I guess since the API uses "gameloops" itself, it would be most consistent to use gameloops also to refer to them. "m_gameloop_frequency" and "m_next_run_gameloop".

Well, if you want to check frames, not count of steps, you should:
a. Keep the actual frames count (or even seconds) in dispatcher.
b. Use modulo on the frames (seconds) count, i.e. call a method from plugin with the current count of passed frames as argument and expect that it returns true or false.
I believe there is no need in storing anything except frequency inside the plugins.

The term frequency means how often a plugin should be called, the actual value against it is tested could be different, but the term itself, imho, very descriptive. There is no way to identify how yours or mine naming approaches work without reading the code, thus I prefer shorter namings.

Well, if you want to check frames, not count of steps, you should:
a. Keep the actual frames count (or even seconds) in dispatcher.
b. Use modulo on the frames (seconds) count, i.e. call a method from plugin with the current count of passed frames as argument and expect that it returns true or false.
I believe there is no need in storing anything except frequency inside the plugins.

b. Modulo does not work for GameLoops(frames) with StepSize >= 1. It will 'miss' frequencies on non-exact GameLoops, leading to longer waits than intended.

StepSize=2 , plugin->m_frequency=3 (Dispatcher D)
GameLoop:0, Step:0, D::OnStep called , 0 % 3 == 0 , plugin->OnStep CALLED
GameLoop:1, Step:-, D::OnStep skipped, 1 % 3 == 1 , plugin->OnStep not checked
GameLoop:2, Step:1, D::OnStep, 2 % 3 == 2 , plugin->OnStep skipped
GameLoop:3, Step:-, skipped, 3 % 3 == 0 , plugin->OnStep not checked <- MISSED FREQUENCY
GameLoop:4, Step:2, D::OnStep, 4 % 3 == 1 , plugin->OnStep skipped
GameLoop:5, Step:-, skipped, 5 % 3 == 2 , plugin->OnStep not checked
GameLoop:6, Step:3 D:::OnStep, 6 % 3 == 0 , plugin->OnStep CALLED (waited 6 GameLoops)

Modulo only works if it is checked on the 'exact' gameloop/frame, therefore missing frequencies on the 'skipped' Dispatcher::OnStep's due to StepSize>=1.

If we use 'Step Frequency' instead of 'GameLoop Frequency', then the tuned timings will be off if the StepSize is ever changed, but maybe that is okay and authors can update the plugins' m_frequencies to match. I will push a commit that does it the 'Step Frequency' way (using m_steps in Dispatcher as you suggested in (a)). Let me know if that is more towards what you want.

Lets reshape this logic a bit:

Please, move all the checking parts into Dispatcher as this idea is closer to it (e.g. consider that we issue onStep event or block it if needed).

If modulo doesn't work well, we can do a bit different approach similar to algorithms used in computer networks (transfer of packets on the network level). Some (or perhaps many) network cards drivers act in similar manner: they count 'time left' for each sent packet and if no response received, packet loss is assumed.

So, instead of determining when a plugin should be called, we can track exactly 'how many turns left for a plugin to be called'.

The scheme is following:
a. Dispatcher has an actual map of 'turns left - a plugin'.
b. Each plugin reports how much time left to call it again on first call of onStep method.
c. Dispatcher fills in the map.
d. Dispatcher reduces each counter in the map each onStep by operating current frames count per step value.
e. If counter becomes <= 0, we call onStep of corresponding plugin, take return values from it and start the countdown again.

Such approach provides the following benefits:

Very precise timer. We use exact number of passed frames.

A plugin can report different frequencies (seems that this the case with Miner), even sleep for e.g. 300 frames at the beginning of a game.

Technically a plugin can ask Dispatcher to increase counter by some reason or reset it, but so far we don't need such functionality.

All control flow is located in single place - Dispatcher.

Please, let me know what do you think about this idea and sorry for so long review.

src/plugins/QuarterMaster.cpp

src/plugins/Plugin.h

alkurbatov

Better, but requires changes.

Only check vespene workers every 7 steps, to prevent too many vespene workers from being added and oversaturated. Also, create functionality for other Plugin-derived classes to run every X steps.

ImpulseCloud changed the title ~~Feature #34: Fix gas saturation~~ Fix #34: Fix gas saturation Mar 16, 2021

ImpulseCloud changed the title ~~Fix #34: Fix gas saturation~~ fix: Fix gas saturation, #34 Mar 19, 2021

ImpulseCloud force-pushed the fix-gas-saturation branch from abb2744 to 9af04ce Compare March 19, 2021 00:26

alkurbatov requested changes Mar 20, 2021

View reviewed changes

ImpulseCloud force-pushed the fix-gas-saturation branch from 9af04ce to 3bbc684 Compare March 21, 2021 03:34

alkurbatov reviewed Mar 21, 2021

View reviewed changes

ImpulseCloud force-pushed the fix-gas-saturation branch from 3bbc684 to 1d4fc36 Compare March 21, 2021 19:18

alkurbatov reviewed Mar 22, 2021

View reviewed changes

src/plugins/Miner.cpp Outdated Show resolved Hide resolved

alkurbatov requested changes Mar 22, 2021

View reviewed changes

src/plugins/Miner.cpp Outdated Show resolved Hide resolved

ImpulseCloud force-pushed the fix-gas-saturation branch from 1d4fc36 to 5052fdd Compare March 28, 2021 19:26

ImpulseCloud force-pushed the fix-gas-saturation branch 2 times, most recently from b4f7403 to 487d28e Compare March 28, 2021 22:15

alkurbatov reviewed Mar 29, 2021

View reviewed changes

src/plugins/QuarterMaster.cpp Outdated Show resolved Hide resolved

alkurbatov reviewed Mar 29, 2021

View reviewed changes

src/plugins/Plugin.h Outdated Show resolved Hide resolved

alkurbatov requested changes Mar 29, 2021

View reviewed changes

ImpulseCloud force-pushed the fix-gas-saturation branch from 487d28e to 18a41b5 Compare March 30, 2021 04:55

fix: Fix gas saturation, alkurbatov#34

fc726bc

Only check vespene workers every 7 steps, to prevent too many vespene workers from being added and oversaturated. Also, create functionality for other Plugin-derived classes to run every X steps.

ImpulseCloud force-pushed the fix-gas-saturation branch from 18a41b5 to fc726bc Compare March 30, 2021 18:03

alkurbatov force-pushed the master branch 2 times, most recently from 75b66e5 to 13305b3 Compare July 18, 2021 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix gas saturation, #34 #60

fix: Fix gas saturation, #34 #60

ImpulseCloud commented Mar 15, 2021

ImpulseCloud commented Mar 19, 2021

alkurbatov Mar 20, 2021

alkurbatov Mar 20, 2021

alkurbatov Mar 20, 2021

alkurbatov Mar 20, 2021

ImpulseCloud Mar 20, 2021

alkurbatov Mar 20, 2021

ImpulseCloud Mar 21, 2021

alkurbatov Mar 21, 2021

ImpulseCloud Mar 21, 2021

ImpulseCloud commented Mar 21, 2021

alkurbatov commented Mar 22, 2021

ImpulseCloud commented Mar 28, 2021 •

edited

alkurbatov Mar 29, 2021 •

edited

ImpulseCloud Mar 30, 2021

alkurbatov Mar 30, 2021

alkurbatov Mar 30, 2021 •

edited

ImpulseCloud Mar 30, 2021

alkurbatov Apr 7, 2021 •

edited

alkurbatov left a comment

		@@ -24,6 +24,8 @@ struct GameObject {

		static sc2::Unit ToUnit(sc2::Tag tag_);

		const sc2::Unit* m_unit;

fix: Fix gas saturation, #34 #60

Are you sure you want to change the base?

fix: Fix gas saturation, #34 #60

Conversation

ImpulseCloud commented Mar 15, 2021

ImpulseCloud commented Mar 19, 2021

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

ImpulseCloud commented Mar 21, 2021

alkurbatov commented Mar 22, 2021

ImpulseCloud commented Mar 28, 2021 • edited

alkurbatov Mar 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alkurbatov Mar 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alkurbatov Apr 7, 2021 • edited

Choose a reason for hiding this comment

alkurbatov left a comment

Choose a reason for hiding this comment

ImpulseCloud commented Mar 28, 2021 •

edited

alkurbatov Mar 29, 2021 •

edited

alkurbatov Mar 30, 2021 •

edited

alkurbatov Apr 7, 2021 •

edited