Garrison Limit #2622

igorkis-scrts · 2023-01-09T13:24:37Z

What type of PR is this.

Bug
Change
Enhancement

What have you changed and why?

Information:
This PR introduces garrison limit for rebels, so they can no longer put excessive amount of troops to guard their precious outposts.
Also, it fixes minor bug with garrisonInfo which could break if any garrisoned unit non-mentioned in function (for example, explosive specialists or engineers) will break the function as there was no default case in switch for other types of units.

Please specify which Issue this PR Resolves.

closes #XXXX

Please verify the following and ensure all checks are completed.

Have you loaded the mission in LAN host?
Have you loaded the mission on a dedicated server?

Is further testing or are further changes required?

No
Yes (Please provide further detail below.)

How can the changes be tested?

Steps:
Add someone to garrison of captured outpost of any type.

killerswin2

Good Groundwork.

A3A/addons/core/Params.hpp

A3A/addons/core/functions/Garrison/fn_getGarrisonLimit.sqf

killerswin2 · 2023-01-10T03:26:11Z

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

+private _limit = [_nearX] call A3A_fnc_getGarrisonLimit;
+private _newGarrison = (count (units _groupX)) + (count (garrison getVariable [_nearX, []]));
+if (_newGarrison > _limit) exitWith {
+	["Garrison", "Adding this squad to garrison will exceed garrison limit."] call A3A_fnc_customHint;


localize/ make this a stringtable entry

killerswin2 · 2023-01-10T03:27:32Z

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

@@ -17,7 +17,7 @@ if (_typeX == FactionGet(reb,"unitCrew")) then {_costs = _costs + ([FactionGet(r

 if (_costs > _resourcesFIA) exitWith {["Garrisons", format ["You do not have enough money for this kind of unit (%1 € needed).",_costs]] call A3A_fnc_customHint;};

-_markerX = positionXGarr;
+private _markerX = positionXGarr;


??? why take that out of the private array?

Bulk private declarations are nothing but code smell and "it was the nineties"-kind of thing. The most common reason to get rid of them is that people are forgetting to clear this private variables array from unused variables since IDE can't highlight unused variables and applying to Antistasi's codebase it's usually a really long string in code listing. Also, declaring all used variables in the beginning of file at the top is weird (what If my code will take exitWith route and will not use 90% of variables?).

Also, they have slightly less performance by itself (per BI wiki, basically private array splits declaration and initialization thus having less performance):

private _a = 1; private _b = 2; private _c = 3; private _d = 4; // 0.0023 ms private ["_a", "_b", "_c", "_d"]; _a = 1; _b = 2; _c = 3; _d = 4; // 0.0040 ms

So I don't see any valid reason to maintain them and they can be slowly removed out of codebase.

I don't think killer would have objected if you'd removed the whole array. If you're not refactoring the whole file but you want to make it locally more readable then it's fair enough to pull these out.

killerswin2 · 2023-01-10T03:27:46Z

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

+private _garrison = garrison getVariable [_markerX,[]];
+
+if (count _previousGarrison >= ([_markerX] call A3A_fnc_getGarrisonLimit)) exitWith {
+	["Garrisons", "Garrison limit has reached, you can't add new units anymore."] call A3A_fnc_customHint;


localize/ make this a stringtable entry

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

killerswin2

Alright that some good work!

jaj22 · 2023-01-10T18:50:21Z

For HC squads, simply refusing to add the squad when it'd exceed the size is problematic because a) it's annoying and b) it may result in players leaving HC squads active rather than cleaning them up, which is frequently a worse performance problem than large garrisons. I'd much rather have a system that added up to the limit and refunded the rest, but this would probably need a yes/cancel dialog so it's a bit more complex.

My other concern is that giving a maximum size will encourage players to aim for it, so you might end up with larger garrisons in practice. An alternative system that I'm inclined towards at the moment is to tax per unit (and per static), so there's always a nudge towards smaller garrisons (and a reason to clean up HC squads) but players can choose to stuff points strategically.

Is this intentionally using the same limit for cities as outposts?

igorkis-scrts · 2023-01-10T19:11:16Z

Is this intentionally using the same limit for cities as outposts?

Yeah, I think that same as outpost value is enough for cities. There are much more cities than airbases, so allowing bigger limit might lead to worse experience (as you have noticed, players likely will try to fully fill their garrisons). Cities are usually targeted by Punishment missions and it will spawn additional civilians with guns, so I think it's more or less fine.

About taxes - I think that players are already pressured enough with the HR income and constant need of troops training. Tax system is interesting but IMO it will punish players a lot more than it probably should ("we hired a lot people, where is muh FIA money, why it's constantly near zero?"). If the limit number is not too high (I would remove 50 value, maybe 40 too), the completionist nature of players will not hurt much.

Valid concern about HC squads. I'll see what can be done there.

Bob-Murphy · 2023-01-10T22:53:25Z

I also would remove 40 and 50 as options.
Tax system could be interesting, but I would add that in a later step.

Bob-Murphy

As you are already working on the files, could you please localize the other strings in fn_addToGarrison.sqf and fn_resourcecheck.sqf? That'd be nice.
Thank you.

Bob-Murphy · 2023-01-10T22:58:31Z

A3A/addons/core/functions/Base/fn_garrisonInfo.sqf

    }) pushBack _x;
 } forEach _garrison;

 _textX = format [
-    "<br/><br/>Garrison men: %1<br/><br/>Squad Leaders: %2<br/>%11: %3<br/>Riflemen: %4<br/>Autoriflemen: %5<br/>Medics: %6<br/>Grenadiers: %7<br/>Marksmen: %8<br/>AT Men: %9<br/>Static Weap: %10"
+    "<br/><br/>Garrison men: %1%13<br/><br/>Squad Leaders: %2<br/>%12: %3<br/>Riflemen: %4<br/>Autoriflemen: %5<br/>Medics: %6<br/>Grenadiers: %7<br/>Marksmen: %8<br/>AT Men: %9<br/>Other: %10<br/>Static Weap: %11"


Change Garrison men to Garrison units

killerswin2 · 2023-01-10T23:30:49Z

Keep 40, you can remove 50

Bob-Murphy · 2023-01-10T23:37:39Z

40 at outposts and every city/village is unreasonable.
If people want more than 30 as base, let them disable it and be fully unreasonable.
If your argument for example is "more at airfields" we can rather adjust the percentages after testing.

killerswin2 · 2023-01-10T23:54:32Z

Bob, it is not us who decides what is unreasonable. Let the players decides by giving them options. There is no harm in having them.

killerswin2 · 2023-01-10T23:55:38Z

case in point, with 30, I can't have two ai squads as limit is 15, not 16.

igorkis-scrts · 2023-01-11T11:20:16Z

I would say that players have a tendency to shoot themselves in the foot and then blaming developers for it, so giving potentionally dangerous instruments to players is a risky move.
Prosumers will fork and make their edits anyway, so some boundaries how it should be played should exist.

igorkis-scrts · 2023-01-11T11:58:06Z

As you are already working on the files, could you please localize the other strings in fn_addToGarrison.sqf and fn_resourcecheck.sqf?

Done with the fn_addToGarrison.sqf only. fn_resourcecheck (and many other cases) has a problem - if Engilsh strings will be just swapped with localize calls, already localized values will be broadcasted across all clients, so players might see not their language messages. It will require separate localization PR with a function that broadcasts "localize call" to clients (fn_commsMP already does that to some extent).

Bob-Murphy · 2023-01-21T12:08:29Z

A3A/addons/GUI/dialogues/ids.inc

@@ -319,6 +319,17 @@ Maintainer: DoomMetal
 #define A3A_IDC_ARSLIMTYPESBASE 9550


+// Arsenal guest limits
+#define A3A_IDD_HCDISMISSALDIALOG 9600
+// #define A3A_IDC_ARSLIMARROWMINUS 9501


What are all this unused defines for, will they be needed in the future or can they be deleted?

Bob-Murphy · 2023-01-21T12:11:09Z

@igorkis-scrts You added GUI/fn_hcDismissalDialog.sqf but that file is completely empty. Is that needed?

Bob-Murphy · 2023-01-21T12:11:58Z

also, merge conflict

igorkis-scrts · 2023-01-21T12:12:28Z

HC squad dismissal dialog is not yet ready, so this PR is still WIP. At this moment it's basically a placeholder dialog.

igorkis-scrts · 2023-01-28T11:47:53Z

PR has been updated with previously discussed changes.

Bob-Murphy · 2023-02-15T11:10:08Z

@jaj22 Please re-check.
Cheers

jaj22

Seems fine. Marked some minor stuff.

A design issue is that cities are using the outpost limit (24 by default) regardless of population, and I think players will be inclined to treat the visible limit as a target. Previously players rarely garrisoned towns, so this may end up hurting their performance by doing what they think is intended.

Would recommend setting city limits according to sqrt population (which lines up with the enemy targeting value), rounding to even numbers to make it look less random.

jaj22 · 2023-02-15T13:02:42Z

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

+        };
+        case (_newGarrisonCount >= _limit): {
+            private _unitsToRefundCount = _newGarrisonCount - _limit;
+            private _unitsToRefund = _unitsX select {!isPlayer _x}; //commander may be amongst the units


isPlayer case is filtered out at line 59. Doesn't much matter, but the comment kinda opens a can of worms if it was possible.

Oh, I was using units group player for testing instead of selected counterpart.

Example 4 for units says there's no use in units group player when units player gives the equal result.

jaj22 · 2023-02-15T13:06:20Z

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

+            for "_i" from 0 to (count _unitsX / 2) do { 
+                private _temp = _unitsX select _i; 
+                private _revertIndex = (count _unitsX) - _i - 1; 
+                _unitsX set [_i, _unitsX select _revertIndex]; 
+                _unitsX set [_revertIndex, _temp]; 
+            };


This is just an array reverse, isn't it? SQF actually has a command for that:
https://community.bistudio.com/wiki/reverse

Could also use deleteRange.

For some reason reverse is not working for units array, it returns nil.

private _unitsX = groupselectedUnits player; reverse _unitsX;

reverse and deleteRange both modify the array in-place and return nothing, so in this case you'd do something like:

private _unitsToRefund = +_unitsX; reverse _unitsToRefund; _unitsToRefund resize _unitsToRefundCount;

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

jaj22 · 2023-02-15T13:29:12Z

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

@@ -17,7 +17,7 @@ if (_typeX == FactionGet(reb,"unitCrew")) then {_costs = _costs + ([FactionGet(r

 if (_costs > _resourcesFIA) exitWith {["Garrisons", format ["You do not have enough money for this kind of unit (%1 € needed).",_costs]] call A3A_fnc_customHint;};

-_markerX = positionXGarr;
+private _markerX = positionXGarr;


I don't think killer would have objected if you'd removed the whole array. If you're not refactoring the whole file but you want to make it locally more readable then it's fair enough to pull these out.

A3A/addons/core/functions/init/fn_resourcecheck.sqf

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

jaj22

All looks good now.

jaj22 · 2023-02-15T19:02:19Z

A3A/addons/core/functions/REINF/fn_addToGarrison.sqf

+            private _unitsToRefund = +_unitsX;
+            reverse _unitsToRefund;
+            _unitsToRefund deleteRange [_unitsToRefundCount, count _unitsX]; 
+            _unitsX deleteRange [(count _unitsX - _unitsToRefundCount), count _unitsX];


...although this is still pretty weird. Maybe the cleanest version:

private _unitsToRefund = _unitsX select [0, _unitsToRefundCount]; _unitsX deleteRange [0, _unitsToRefundCount];

Bob-Murphy

Some small and nice-to-have things.

Bob-Murphy · 2023-02-16T09:17:18Z

A3A/addons/core/Stringtable.xml

+        <Russian>Достигнут лимит гарнизона, вы больше не можете добавлять в него новые юниты.</Russian>
+      </Key>
+      <Key ID="STR_A3A_garrison_select_zone">
+          <Original>Select the zone on where you will send the selected troops as garrison.</Original>


doubled indentation on the languages (Original and Russian) starting here and below.

Bob-Murphy · 2023-02-16T09:17:46Z

A3A/addons/core/functions/Base/fn_garrisonInfo.sqf

    }) pushBack _x;
 } forEach _garrison;

 _textX = format [
-    "<br/><br/>Garrison men: %1<br/><br/>Squad Leaders: %2<br/>%11: %3<br/>Riflemen: %4<br/>Autoriflemen: %5<br/>Medics: %6<br/>Grenadiers: %7<br/>Marksmen: %8<br/>AT Men: %9<br/>Static Weap: %10"
+    "<br/><br/>Garrison units: %1%13<br/><br/>Squad Leaders: %2<br/>%12: %3<br/>Riflemen: %4<br/>Autoriflemen: %5<br/>Medics: %6<br/>Grenadiers: %7<br/>Marksmen: %8<br/>AT Men: %9<br/>Other: %10<br/>Static Weap: %11"


Can we localize this fucker?

I wouldn't recommend it. That's going to be one hell of an ugly XML string, and I'm pretty sure it's redundant in the new UI.

Bob-Murphy · 2023-02-16T09:22:54Z

A3A/addons/core/Stringtable.xml

+          <Russian>Выберите зону, куда вы отправите выбранные войска в качестве гарнизона.</Russian>
+      </Key>
+      <Key ID="STR_A3A_garrison_fail_not_markerzone">
+          <Original>You must select at any %1 zone with marker.</Original>


You must select at any %1 zone with marker. sounds a bit wrong. I think the at can be deleted.

Bob-Murphy · 2023-02-16T09:24:05Z

A3A/addons/core/Stringtable.xml

+        <Russian>Этот гарнизон полон, выберите другую локацию для того чтобы добавить этот отряд в гарнизон.</Russian>
+      </Key>
+      <Key ID="STR_A3A_garrison_exceed_limit">
+        <Original>Adding this squad to garrison will exceed garrison limit. Some part of them will join garrison, the rest will be dismissed and their cost will be refunded.</Original>


Some part of them will join garrison, --> Some of them will join the garrison,

Bob-Murphy · 2023-02-16T09:27:55Z

A3A/addons/core/Stringtable.xml

+          <Russian>Добавление отряда %1 в гарнизон...</Russian>
+      </Key>
+      <Key ID="STR_A3A_garrison_fail_zone_lost">
+          <Original>%1 group is back to HC control because the zone which was pointed to garrison has been lost.</Original>


%1 group is back to HC control because the zone which was pointed to garrison has been lost. --> %1 group has returned to HC control as the zone which was planned to be garrisoned has been lost.

Bob-Murphy · 2023-02-16T09:34:35Z

A3A/addons/core/functions/REINF/fn_garrisonAdd.sqf

@@ -27,38 +27,23 @@ if (surfaceIsWater _positionX) exitWith {["Garrisons", "This Garrison is still u

 if ([_positionX] call A3A_fnc_enemyNearCheck) exitWith {["Garrisons", "You cannot Recruit Garrison Units with enemies near the zone."] call A3A_fnc_customHint;};


Please localize this as well as all the others in this file, as we are already working on it.
Certainly makes it easier for the localisation as a whole.
Thank you.

# Conflicts: # A3A/addons/core/Stringtable.xml

Bob-Murphy

lgtm

* garrison limit func * marker parameter * usage of garrison limit plus fixed bug with non-mentioned unit types * usage of garrison limit * additional values * PR tweaks and fixes * new factor of 8 values * garrison men->units * tooltip support * localization plus light refactoring, error fix * localization strings * .idea gitignore * dismissal dialog (WIP) * removal of HC dismissal dialog * localization strings * excessive troops refund * refund rewrite * redunant marker updates * garrison add pr fixes * tweaks and fixes * tweak * stringtable fixes * localization, slight refactoring * localization fix

igorkis-scrts added 4 commits January 9, 2023 15:35

garrison limit func

74b59ab

marker parameter

738a21c

usage of garrison limit plus fixed bug with non-mentioned unit types

9054497

usage of garrison limit

a8787d8

killerswin2 requested changes Jan 10, 2023

View reviewed changes

igorkis-scrts added 2 commits January 10, 2023 12:04

additional values

6c933ad

PR tweaks and fixes

94910b2

killerswin2 approved these changes Jan 10, 2023

View reviewed changes

Bob-Murphy reviewed Jan 10, 2023

View reviewed changes

igorkis-scrts added 3 commits January 11, 2023 14:24

new factor of 8 values

4737e7c

garrison men->units

4f3a756

tooltip support

9a8a38e

igorkis-scrts added 4 commits January 11, 2023 15:07

localization plus light refactoring, error fix

d42fdc8

localization strings

db1ace2

.idea gitignore

cafa629

dismissal dialog (WIP)

ecaff69

Bob-Murphy added this to the 3.2 milestone Jan 11, 2023

Bob-Murphy reviewed Jan 21, 2023

View reviewed changes

Bob-Murphy requested a review from jaj22 February 1, 2023 13:21

Bob-Murphy added Review pending and removed Work needed labels Feb 15, 2023

jaj22 requested changes Feb 15, 2023

View reviewed changes

igorkis-scrts added 3 commits February 15, 2023 20:50

redunant marker updates

0c15f1e

garrison add pr fixes

43024ab

tweaks and fixes

1cf7fd8

igorkis-scrts requested a review from jaj22 February 15, 2023 18:33

jaj22 approved these changes Feb 15, 2023

View reviewed changes

jaj22 reviewed Feb 15, 2023

View reviewed changes

tweak

13c4fa6

jaj22 added Ready for merge and removed Review pending labels Feb 15, 2023

Bob-Murphy requested changes Feb 16, 2023

View reviewed changes

Bob-Murphy added Change requested A change has been requested, and this can't be merged until it's done. and removed Ready for merge labels Feb 16, 2023

igorkis-scrts added 2 commits February 16, 2023 20:55

stringtable fixes

8d3c325

localization, slight refactoring

27c07b3

igorkis-scrts requested a review from Bob-Murphy February 16, 2023 18:05

igorkis-scrts added 2 commits February 17, 2023 00:08

localization fix

1a7261a

Merge branch 'upstream-unstable' into feature/garrison-limit-port

2299cb8

# Conflicts: # A3A/addons/core/Stringtable.xml

Bob-Murphy approved these changes Mar 12, 2023

View reviewed changes

Bob-Murphy merged commit b9ea7a3 into official-antistasi-community:unstable Mar 12, 2023

igorkis-scrts deleted the feature/garrison-limit-port branch March 12, 2023 07:33

Bob-Murphy added Ready for merge and removed Change requested A change has been requested, and this can't be merged until it's done. labels Mar 26, 2023

Bob-Murphy added the Added to changelog Added to changelog label May 8, 2023

		@@ -27,38 +27,23 @@ if (surfaceIsWater _positionX) exitWith {["Garrisons", "This Garrison is still u

		if ([_positionX] call A3A_fnc_enemyNearCheck) exitWith {["Garrisons", "You cannot Recruit Garrison Units with enemies near the zone."] call A3A_fnc_customHint;};

Garrison Limit #2622

Garrison Limit #2622

Conversation

igorkis-scrts commented Jan 9, 2023

What type of PR is this.

What have you changed and why?

Please specify which Issue this PR Resolves.

Please verify the following and ensure all checks are completed.

Is further testing or are further changes required?

How can the changes be tested?

killerswin2 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

killerswin2 left a comment

Choose a reason for hiding this comment

jaj22 commented Jan 10, 2023

igorkis-scrts commented Jan 10, 2023 • edited

Bob-Murphy commented Jan 10, 2023

Bob-Murphy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killerswin2 commented Jan 10, 2023

Bob-Murphy commented Jan 10, 2023 • edited

killerswin2 commented Jan 10, 2023 • edited

killerswin2 commented Jan 10, 2023

igorkis-scrts commented Jan 11, 2023

igorkis-scrts commented Jan 11, 2023 • edited

Choose a reason for hiding this comment

Bob-Murphy commented Jan 21, 2023

Bob-Murphy commented Jan 21, 2023

igorkis-scrts commented Jan 21, 2023

igorkis-scrts commented Jan 28, 2023

Bob-Murphy commented Feb 15, 2023

jaj22 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

igorkis-scrts Feb 15, 2023 • edited

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

jaj22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bob-Murphy 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

Bob-Murphy Feb 16, 2023 • edited

Choose a reason for hiding this comment

Bob-Murphy left a comment

Choose a reason for hiding this comment

igorkis-scrts commented Jan 10, 2023 •

edited

Bob-Murphy commented Jan 10, 2023 •

edited

killerswin2 commented Jan 10, 2023 •

edited

igorkis-scrts commented Jan 11, 2023 •

edited

igorkis-scrts Feb 15, 2023 •

edited

Bob-Murphy Feb 16, 2023 •

edited