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 ReloadAmmoPool trait #13538

Merged
merged 7 commits into from Nov 12, 2017

Conversation

@reaperrr
Contributor

reaperrr commented Jun 23, 2017

First step towards #9613.

This adds a ReloadAmmoPool trait, splits the AmmoPool reload logic into a method that can be called externally, and gets rid of the AmmoPool.SelfReloads-related properties.

The 3rd commit looks more intimidating than it really is, but if push comes to shove, I can split it off to a follow-up PR along the upgrade rule.

Note: I've tested the upgrade rule myself - even on a testcase with multiple AmmoPools - and verified that it works as intended.

@reaperrr reaperrr added this to the Next release milestone Jun 23, 2017

@reaperrr reaperrr changed the title from Add ReloadAmmoPoolOnCondition to Add ReloadAmmoPool trait Jun 23, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 23, 2017

Contributor

Updated.

Contributor

reaperrr commented Jun 23, 2017

Updated.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Jul 14, 2017

Member

Needs a rebase.

Member

abcdefg30 commented Jul 14, 2017

Needs a rebase.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 20, 2017

Contributor

Rebased.

Contributor

reaperrr commented Jul 20, 2017

Rebased.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 17, 2017

Contributor

Rebased.

Contributor

reaperrr commented Aug 17, 2017

Rebased.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Rebased, added RulesetLoaded check and fixed upgrade rule.

Contributor

reaperrr commented Sep 10, 2017

Rebased, added RulesetLoaded check and fixed upgrade rule.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 22, 2017

Contributor

Updated and rebased.

Contributor

reaperrr commented Sep 22, 2017

Updated and rebased.

@pchote

Sorry, I have another low-level architecture issue. I don't think this should be too hard to untangle though, and it should lead to a much cleaner implementation in the long run (after Rearm is finally removed).

Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 15, 2017

Contributor

Updated (and rebased).

Contributor

reaperrr commented Oct 15, 2017

Updated (and rebased).

@reaperrr reaperrr removed the PR: Rebase me! label Nov 4, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 9, 2017

Contributor

@pchote @penev92 This is blocking #14036 + follow-up as well as the removal of the Rearm activity, both of which I want to get into Next+1, so this shouldn't sit unmerged for too much longer, if possible.

Contributor

reaperrr commented Nov 9, 2017

@pchote @penev92 This is blocking #14036 + follow-up as well as the removal of the Rearm activity, both of which I want to get into Next+1, so this shouldn't sit unmerged for too much longer, if possible.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 9, 2017

Member

Sorry @reaperrr, i've been on a work trip for the last two weeks and have had basically zero time for OpenRA. Will be back on on Sunday and can start towards clearing the backlog.

Member

pchote commented Nov 9, 2017

Sorry @reaperrr, i've been on a work trip for the last two weeks and have had basically zero time for OpenRA. Will be back on on Sunday and can start towards clearing the backlog.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 9, 2017

Contributor

Sorry @reaperrr, i've been travelling for work for the last two weeks and have had basically zero time for OpenRA.

I know, just wanted to make sure this doesn't go under the radar since it's so far back in the PR list by now.

Contributor

reaperrr commented Nov 9, 2017

Sorry @reaperrr, i've been travelling for work for the last two weeks and have had basically zero time for OpenRA.

I know, just wanted to make sure this doesn't go under the radar since it's so far back in the PR list by now.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 9, 2017

Contributor

Updated, fixed that only 1 instance would be granted at creation, regardless of initial/max ammo.

Contributor

reaperrr commented Nov 9, 2017

Updated, fixed that only 1 instance would be granted at creation, regardless of initial/max ammo.

@penev92

General questions:

  1. Once this refactoring is done, will airfields be able to increase the speed at which ReloadAmmoPool reloads "self-reloading" AmmoPools?
  2. Will airfields be able to pause ReloadAmmoPool traits in order to reload the AmmoPools at an overall different speed?
    (Rearm seems to be intentionally ignoring auto-reloading AmmoPools, which I bet is a bit limiting in terms of gameplay possibilities for modders.)
Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
public bool GiveAmmo()
public bool GiveAmmo(Actor self, int count)

This comment has been minimized.

@penev92

penev92 Nov 11, 2017

Member

Since we have the Reload() method below and the scripting interface use this, and Rearm and ReloadAmmoPool use the below Reload() (which is 100% identical to this, since it just calls it),
either this should become private and only be called from Reload(),
or Reload() should be removed in favour of this.

P.S.: Or remove this and move the code to Reload().

@penev92

penev92 Nov 11, 2017

Member

Since we have the Reload() method below and the scripting interface use this, and Rearm and ReloadAmmoPool use the below Reload() (which is 100% identical to this, since it just calls it),
either this should become private and only be called from Reload(),
or Reload() should be removed in favour of this.

P.S.: Or remove this and move the code to Reload().

This comment has been minimized.

@GraionDilach

GraionDilach Nov 12, 2017

Contributor

GiveAmmo should stay, to be consistent with TakeAmmo.

@GraionDilach

GraionDilach Nov 12, 2017

Contributor

GiveAmmo should stay, to be consistent with TakeAmmo.

This comment has been minimized.

@penev92

penev92 Nov 12, 2017

Member

Good point. And since TakeAmmo is public, I'd say get rid of Reload.

@penev92

penev92 Nov 12, 2017

Member

Good point. And since TakeAmmo is public, I'd say get rid of Reload.

This comment has been minimized.

@reaperrr

reaperrr Nov 12, 2017

Contributor

I'd say get rid of Reload.

Done.

@reaperrr

reaperrr Nov 12, 2017

Contributor

I'd say get rid of Reload.

Done.

return false;
--CurrentAmmo;
currentAmmo = (currentAmmo - count).Clamp(0, Info.Ammo);

This comment has been minimized.

@penev92

penev92 Nov 11, 2017

Member

Clamping here shows you foresaw a case where an actor would need to fire 5 bullets but have only 3.
What if someone wants to disable firing if there is insufficient ammo left?
I see this PR has been through a lot already, but this could use something like

public bool TakeAmmo(Actor self, int count, bool requiresFullCount)
{
	if (currentAmmo <= 0 || count < 0)
		return false;

	if (currentAmmo < count && requiresFullCount)
		return false;

		...

Then someone (probably AmmoPool? Armament? Weapon? I have no idea) can have a property so that can be controlled from YAML (or Lua, for the one case when the caller is the scripting interface).

P.S.: Upon further inspection it turned out that we have 2 of the 3 cases hardcoded to 1 ammo (AmmoPool.Attacking() and LayMines.LayMine()), so that just leaves the scripting interface as a potential issue, but I'd still like to see that check.

@penev92

penev92 Nov 11, 2017

Member

Clamping here shows you foresaw a case where an actor would need to fire 5 bullets but have only 3.
What if someone wants to disable firing if there is insufficient ammo left?
I see this PR has been through a lot already, but this could use something like

public bool TakeAmmo(Actor self, int count, bool requiresFullCount)
{
	if (currentAmmo <= 0 || count < 0)
		return false;

	if (currentAmmo < count && requiresFullCount)
		return false;

		...

Then someone (probably AmmoPool? Armament? Weapon? I have no idea) can have a property so that can be controlled from YAML (or Lua, for the one case when the caller is the scripting interface).

P.S.: Upon further inspection it turned out that we have 2 of the 3 cases hardcoded to 1 ammo (AmmoPool.Attacking() and LayMines.LayMine()), so that just leaves the scripting interface as a potential issue, but I'd still like to see that check.

This comment has been minimized.

@reaperrr

reaperrr Nov 12, 2017

Contributor

The stacked AmmoCondition allows to disable/pause the attack- or armament trait(s) until the ammo pool is completely filled, so this shouldn't be necessary.

@reaperrr

reaperrr Nov 12, 2017

Contributor

The stacked AmmoCondition allows to disable/pause the attack- or armament trait(s) until the ammo pool is completely filled, so this shouldn't be necessary.

This comment has been minimized.

@reaperrr

reaperrr Nov 12, 2017

Contributor

Clamping here shows you foresaw a case where an actor would need to fire 5 bullets but have only 3.

More like @pchote requested clamping in general as safe-guard against modders messing around too much causing crashes (which makes sense), iirc. I didn't foresee anything, just tried to adress all requests so this damn thing gets merged at some point...

@reaperrr

reaperrr Nov 12, 2017

Contributor

Clamping here shows you foresaw a case where an actor would need to fire 5 bullets but have only 3.

More like @pchote requested clamping in general as safe-guard against modders messing around too much causing crashes (which makes sense), iirc. I didn't foresee anything, just tried to adress all requests so this damn thing gets merged at some point...

return false;
++CurrentAmmo;
currentAmmo = (currentAmmo + count).Clamp(0, Info.Ammo);
UpdateCondition(self);

This comment has been minimized.

@penev92

penev92 Nov 11, 2017

Member

Looks like this needs to happen for (var i = 0; i < count; ... ? Like in INotifiedCreated.Created().

@penev92

penev92 Nov 11, 2017

Member

Looks like this needs to happen for (var i = 0; i < count; ... ? Like in INotifiedCreated.Created().

This comment has been minimized.

@reaperrr

reaperrr Nov 12, 2017

Contributor

Adressed in UpdateCondition instead.

@reaperrr

reaperrr Nov 12, 2017

Contributor

Adressed in UpdateCondition instead.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Activities/Rearm.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/ReloadAmmoPool.cs Outdated
@@ -45,96 +48,119 @@ public class AmmoPoolInfo : ITraitInfo
[Desc("Time to reload per ReloadCount on airfield etc.")]

This comment has been minimized.

@penev92

penev92 Nov 11, 2017

Member

You should add // HACK: Temporarily needed until Rearm activity is gone for good here as well, for good measure.

@penev92

penev92 Nov 11, 2017

Member

You should add // HACK: Temporarily needed until Rearm activity is gone for good here as well, for good measure.

This comment has been minimized.

@reaperrr

reaperrr Nov 12, 2017

Contributor

Done.

@reaperrr

reaperrr Nov 12, 2017

Contributor

Done.

reaperrr added some commits Nov 12, 2017

Fix official mods Aircraft.RearmBuildings setups
Only aircraft that a) have an AmmoPool and b) don't auto-reload should define RearmBuildings.
Add ReloadAmmoPool and adapt AmmoPool
Refactored and simplified Rearm activity.
Uses local Reload now.

Removed AmmoPool.SelfReloads.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 12, 2017

Contributor

Updated.


General questions:

    Once this refactoring is done, will airfields be able to increase the speed at which ReloadAmmoPool reloads "self-reloading" AmmoPools?
    Will airfields be able to pause ReloadAmmoPool traits in order to reload the AmmoPools at an overall different speed?
    (Rearm seems to be intentionally ignoring auto-reloading AmmoPools, which I bet is a bit limiting in terms of gameplay possibilities for modders.)

Both 1. and 2.: Yes, by making the airfield provide a condition that disables (or pauses) the "self-reloading" ReloadAmmoPool and enables another with faster reload speed and/or higher reload count.

Contributor

reaperrr commented Nov 12, 2017

Updated.


General questions:

    Once this refactoring is done, will airfields be able to increase the speed at which ReloadAmmoPool reloads "self-reloading" AmmoPools?
    Will airfields be able to pause ReloadAmmoPool traits in order to reload the AmmoPools at an overall different speed?
    (Rearm seems to be intentionally ignoring auto-reloading AmmoPools, which I bet is a bit limiting in terms of gameplay possibilities for modders.)

Both 1. and 2.: Yes, by making the airfield provide a condition that disables (or pauses) the "self-reloading" ReloadAmmoPool and enables another with faster reload speed and/or higher reload count.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/AmmoPool.cs Outdated
Show outdated Hide outdated mods/cnc/rules/aircraft.yaml Outdated

PR updated, all requests adressed in some way

@pchote

pchote approved these changes Nov 12, 2017

Everything still seems to work with some basic testing. This has dragged on long enough - any remaining issues can be resolved in followups.

@pchote pchote merged commit d602ec6 into OpenRA:bleed Nov 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment