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

Add garrison zeus modules #4555

Merged
merged 25 commits into from
Oct 10, 2017
Merged

Conversation

alganthe
Copy link
Contributor

When merged this pull request will:

  • Add garrison and un-garrison functions to common.
  • Add a garrison module.
  • Add an un-garrison module.

@alganthe alganthe changed the title Add garrison modules Add garrison zeus modules Oct 16, 2016
@jonpas jonpas added the kind/feature Release Notes: **ADDED:** label Oct 16, 2016
@jonpas jonpas added this to the 3.9.0 milestone Oct 16, 2016
Copy link
Member

@kymckay kymckay left a comment

Choose a reason for hiding this comment

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

A quick cursory review, looks as though you've figured out the right module structure for UI stuff 👍

<French>Aucune unité fourni</French>
</Key>
<Key ID="STR_ACE_Common_GarrisonNotEnoughPos">
<English>There isn't enough positions to place all units.</English>
Copy link
Member

Choose a reason for hiding this comment

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

aren't

private _unit = (attachedTo _logic);
private _building = nearestBuilding (getPosASL _unit);

// Handles errors
Copy link
Member

Choose a reason for hiding this comment

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

Validation of the module target should only be taking place in the ui function (it doesn't make sense to show zeus a UI and then tell them they placed the module wrong only after they close it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Only true if the zeus interface updates in real time. I think it's fine to have the check in both

Copy link
Member

Choose a reason for hiding this comment

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

Actually, good point. The unit could be deleted between the UI opening and closing. Can safely ignore my initial comment 👍

curatorInfoType = QGVAR(RscGarrison);
icon = QPATHTOF(UI\Icon_Module_Zeus_Garrison_ca.paa);
};
class GVAR(moduleUnGarrison): GVAR(moduleBase) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if I like having a separate module to undo the effects of another, could combine these into 1 by having a UI option to un-garrison - that way there's a single module that handles all things garrison.

Though that's just a thought, very much unsure what works better in terms of UX (if anyone else has thoughts on this please chime in).

Copy link
Member

Choose a reason for hiding this comment

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

We should decide with what and stick with it. Currently there is already add/remove Virtual Arsenal which is 2 modules. Can always streamline into 1 later if it would be better.

@@ -46,6 +46,7 @@ PREP(fixCollision);
PREP(fixFloating);
PREP(fixLoweredRifleAnimation);
PREP(fixPosition);
PREP(garrison);
Copy link
Member

Choose a reason for hiding this comment

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

Unsure if these functions belong in common or not 😝

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say ace_ai, despite it not having any sqf atm.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't CBA now have garrison functions that could be used instead? If not, then yeah, those should go into ace_ai component.

if (local _x) then {
_x doMove ((nearestBuilding (getPos _x)) buildingExit 0);
} else {
[QGVAR(doMove), [_x, ((nearestBuilding (getPos _x)) buildingExit 0)], _x] call CBA_fnc_targetEvent;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than sending an event for every unit from within this function, you should be running this whole function via an event from the module function.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Also to the note above:

CBA_fnc_targetEvent will send the event to every machine where at least one element of the array is local. If there are object with different localities in the array, it will execute the commands on each of those machines for every object, even if remote. But I guess that would still work for these.

_x enableAI "PATH";
} else {
[QGVAR(enableAI), [_placedUnits, "AUTOCOMBAT"], _placedUnits] call CBA_fnc_targetEvent;
[QGVAR(enableAI), [_placedUnits, "PATH"], _placedUnits] call CBA_fnc_targetEvent;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this is being done inside this loop (also this variable doesn't exist).

@commy2
Copy link
Contributor

commy2 commented Oct 17, 2016

pushback should be pushBack, alganthe :)

@alganthe
Copy link
Contributor Author

alganthe commented Oct 19, 2016

I made some changes related to the unGarrison function, it's called as an event via the module and should only run where the units are local thus I removed the disableAI, follow, and doMove events I had added.

Regarding moving it to the AI component just tell me if I should and i'll do it.

@kymckay kymckay self-assigned this Nov 15, 2016
@PabstMirror PabstMirror modified the milestones: 3.9.0, 3.10.0 Feb 11, 2017
@PabstMirror PabstMirror modified the milestones: 3.11.0, 3.10.0 Jun 2, 2017
* 0: The building(s) nearest this position are used <POSITION>
* 1: Limit the building search to those type of building <ARRAY>
* 2: Units that will be garrisoned <ARRAY>
* 3: Radius to fill building(s) <SCALAR> default: 50
Copy link
Member

Choose a reason for hiding this comment

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

> (default: 50)
In other places too.

* 5: True to fill building(s) from top to bottom <BOOL> default: false

* Return Value:
* Array of units not garrisoned
Copy link
Member

Choose a reason for hiding this comment

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

* Units not garrisoned <ARRAY>

@@ -0,0 +1,39 @@
/*
* Author: alganthe
* Used to un-garrison units
Copy link
Member

Choose a reason for hiding this comment

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

Missing final dot.

@@ -0,0 +1,62 @@
/*
* Author: alganthe
* Module calling the garrison function
Copy link
Member

Choose a reason for hiding this comment

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

Missing final dot.

* 1: Position of the module <POSITION>
* 2: Radius of the task <NUMBER>
* 3: Filling mode of the garrison function <NUMBER>
* 4: Enable or not top down filling <BOOLEAN>
Copy link
Member

Choose a reason for hiding this comment

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

<BOOL>

@@ -0,0 +1,50 @@
/*
* Author: alganthe
* Un-garrison a garrisoned group
Copy link
Member

Choose a reason for hiding this comment

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

Missing final dot.

@@ -0,0 +1,103 @@
/*
* Author: alganthe
* Initalises the `garrison` zeus module display
Copy link
Member

Choose a reason for hiding this comment

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

Missing final dot.

* Initalises the `garrison` zeus module display
*
* Arguments:
* 0: garrison controls group <CONTROL>
Copy link
Member

Choose a reason for hiding this comment

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

Garrison

* 0: garrison controls group <CONTROL>
*
* Return Value:
* Nothing
Copy link
Member

Choose a reason for hiding this comment

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

* None

@jonpas
Copy link
Member

jonpas commented Sep 8, 2017

@SilentSpike ?

@jonpas jonpas requested a review from kymckay September 17, 2017 22:29
} else {
_buildings = nearestObjects [_startingPos, _buildingTypes, _fillingRadius];
_buildings = _buildings call BIS_fnc_arrayShuffle;
};
Copy link
Member

Choose a reason for hiding this comment

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

This whole thing would be neater as:

private _buildings = nearestObjects [_startingPos, _buildingTypes, _fillingRadius];
if (_fillingRadius >= 50) then {
    _buildings = _buildings call BIS_fnc_arrayShuffle;
};

There is also CBA_fnc_shuffle that is probably faster than BI's function.

};
};
} foreach _units;

Copy link
Member

Choose a reason for hiding this comment

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

A line too much.

};

private _units = units _unit;
// Make sure all units are disembarked.
Copy link
Member

Choose a reason for hiding this comment

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

No final dot.

* Module calling the garrison function.
*
* Arguments:
* 0: The module logic <OBJECT>
Copy link
Member

Choose a reason for hiding this comment

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

The is superfluous.

* Un-garrison a garrisoned group.
*
* Arguments:
* 0: The module logic <OBJECT>
Copy link
Member

Choose a reason for hiding this comment

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

The is superfluous.

@@ -0,0 +1,103 @@
/*
* Author: alganthe
* Initalises the `Garrison` zeus module display.
Copy link
Member

Choose a reason for hiding this comment

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

"Garrison"

* Initalises the `Garrison` zeus module display.
*
* Arguments:
* 0: garrison controls group <CONTROL>
Copy link
Member

Choose a reason for hiding this comment

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

Garrison ...

@jonpas jonpas dismissed kymckay’s stale review September 21, 2017 18:50

Requested changes addressed.

@alganthe
Copy link
Contributor Author

This is still very slightly WIP, I have an issue (units are sometimes stack ontop of eachothers when the function is called multiple time), thus I enabled some debug elements.

@alganthe
Copy link
Contributor Author

alganthe commented Sep 25, 2017

AI stacking on rare occasion has been fixed, debug disabled this is now ready for review.

setPos is pretty imprecise, moving units can end up within 2m of the position if they are moving at full running speed, I cannot fix this without inducing some more network usage which I don't want.

Edit: apparently doStop is AG EG, it reduces setPos offset on fast moving units quite nicely, there is still a 1m radius when placed but it should do the trick.

@alganthe
Copy link
Contributor Author

As @jonpas mentionned in PM zeus shouldn't teleport units by default, I'll add a "teleport" button like the top-down filling one in the dialog.

When ticked it will behave like it does right now, otherwise it will use CBA's garrison, my ungarrison module works on both (it's also needed because you have no other way of freeing units garrisoned via the defend module we already have.)

@@ -1112,6 +1112,10 @@
<English>Random filling</English>
<French>Remplir au hasard</French>
</Key>
<Key ID="STR_ACE_Zeus_ModuleGarrison_TeleportText">
Copy link
Member

Choose a reason for hiding this comment

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

<English>Teleport to Base</English>
<Czech>Teleportovat na Základnu</Czech>
<German>Zur Basis teleportieren</German>
<Spanish>Treletransportar a Base</Spanish>
<Polish>Teleport do bazy</Polish>
<Russian>Телепортироваться на базу</Russian>
<French>Téléportation à la base</French>
<Portuguese>Teletransportar para a Base</Portuguese>
<Hungarian>Bázisra teleportálás</Hungarian>
<Italian>Teleporta alla base</Italian>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest?
That I take the first word of the string? It could be different on asian languages.

Copy link
Member

Choose a reason for hiding this comment

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

Just that we have some of them, I have no clue which are correct without the "base" part as well, might as well just leave it.

@alganthe
Copy link
Contributor Author

Okay, this should be ready for review.

@alganthe
Copy link
Contributor Author

Outside of some situation where the AI completely refuses to path this should now work quite nicely now.

Copy link
Contributor

@PabstMirror PabstMirror left a comment

Choose a reason for hiding this comment

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

Tested, works very good

Just fix the little locality issue (throws script error on other machines) and we can merge

#include "script_component.hpp"

params ["_logic"];

Copy link
Contributor

Choose a reason for hiding this comment

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

if (!local _logic) exitWith {}; // Module is global

@PabstMirror PabstMirror merged commit bb03f55 into acemod:master Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Release Notes: **ADDED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants