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

Rearm limited ammo truck munition #3431

Merged
merged 18 commits into from
Jun 8, 2017
Merged

Conversation

GitHawk
Copy link
Contributor

@GitHawk GitHawk commented Feb 26, 2016

The rearm module will be expanded by a new option to select the ammunition amount in an ammo truck.
The two new availability levels are:

  • Amount based on caliber
  • Certain magazines and specific amounts
    The current "unlimited" level will remain as default value.

Supply amount based on caliber means you can for example take either two .50cal magazines or a single 120mm magazine, but not a 155mm magazine when the amount is on the level of 154. So everything can be rearmed but with a limited supply, while bigger calibers take more supply.

Supply amount with certain magazines means you are required to add certain magazines to an ammo truck. No other weapons/magazines will be able to be resupplied.

  • Implement the two levels
  • Add API
  • Testing

Documentation for other modders/creators on how to enable refuel will be in a different PR

@jonpas jonpas added the kind/enhancement Release Notes: **IMPROVED:** label Feb 26, 2016
@jonpas jonpas modified the milestones: 3.5.0, 3.5.1 Feb 26, 2016
@jonpas
Copy link
Member

jonpas commented Feb 26, 2016

I'd wait with the module additions until we decide what to do with settings/Eden attributes.

} else {
EGVAR(common,remoteFnc) = [_this, QFUNC(rearmSuccessLocal), 0];
_turretOwnerID publicVariableClient QEGVAR(common,remoteFnc);
[QGVAR(rearmSuccessLocalEH), _turretOwnerID, _this] call EFUNC(common,targetEvent);
};
} else {
[_this, QFUNC(rearmSuccess), 1] call EFUNC(common,execRemoteFnc);
Copy link
Member

Choose a reason for hiding this comment

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

As discussed on Slack, use EFUNC(common,serverEvent) here and in postInit add the event in isServer only.

@thojkooi thojkooi modified the milestones: 3.6.0, 3.5.1 Mar 7, 2016
@thojkooi
Copy link
Contributor

thojkooi commented May 7, 2016

Has this been reviewed? It's a significant amount of code.

@GitHawk
Copy link
Contributor Author

GitHawk commented May 7, 2016

No, I tested it a lot while writing it. That was 2 ArmA versions ago :-(

Made params nicer, replaced some forEach and used params in XEH_respawn
@jonpas
Copy link
Member

jonpas commented May 25, 2016

@GitHawk any further testing on this?

* Display a structured text.
*
* Arguments:
* 0: Text <ANY>
* 1: Size of the textbox (default: 1.5) <NUMBER>
* 2: Target Unit. Will only display if target is the player controlled object (default: ACE_player) <OBJECT>
* 3: Custom Width <NUMBER><OPTIONAL>
Copy link
Member

@jonpas jonpas Jun 6, 2016

Choose a reason for hiding this comment

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

Why is that necessary? You can just calculate the size based on length of the string, not sure I like this.

Either way, that line should be like:

 * 3: Custom Width <NUMBER> (default: 10)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In its original way, you couldn't manipulate the width at all.So in order to break less, I added an optional parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it doesn't hurt but it will make it inconsistent with other hints. @commy2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it won't. The default value is the old fixed value and since it's optional nothing else is broken.

];

if (isNull _truck ||
{_magazineClass isEqualTo ""}) exitWith {false};
Copy link
Member

Choose a reason for hiding this comment

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

Same line.

@GitHawk
Copy link
Contributor Author

GitHawk commented Jun 7, 2016

Since the first merge broke a lot of stuff, everything should be fine now.

@bux
Copy link
Member

bux commented Jun 7, 2016

Why is this branch both change of headers and the rearm modification?
Imo it should be two separate branches, to better keep track of the changes.
Would this be possible to do for you @GitHawk

@GitHawk
Copy link
Contributor Author

GitHawk commented Jun 7, 2016

Unfortunately no. I made this PR in February. In June it gets reviewed. Some changed to headers are suggested, as they changed since. To avoid me and others from making the same (copy paste) error again and again, I replaced all headers that were "wrong".

Also I'm getting sick and tired of keeping my month-old PRs up to date. If you want some changes made, please PR me on my branch.

@jonpas
Copy link
Member

jonpas commented Jun 7, 2016

We have priorities, and this is a large change in code
Additionally this can't get merged before change to CBA events is, so it will be even more time before we'll be able to merge it. Sorry for inconvenience.

@jonpas
Copy link
Member

jonpas commented Jun 11, 2016

Will re-PR this in multiple PRs appropriately.

@jonpas
Copy link
Member

jonpas commented Jun 18, 2016

Separated into #3943 and #3944.

I won't be doing API things, if you can please re-PR that @GitHawk.

@jonpas jonpas closed this Jun 18, 2016
@jonpas jonpas removed this from the 3.6.0 milestone Jun 18, 2016
@jonpas jonpas removed their assignment Jun 18, 2016
@PabstMirror PabstMirror reopened this May 20, 2017
@PabstMirror PabstMirror merged commit 8e00474 into acemod:master Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants