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 Rearmable trait #14357

Merged
merged 3 commits into from Nov 2, 2018

Conversation

Projects
None yet
6 participants
@reaperrr
Copy link
Contributor

reaperrr commented Nov 14, 2017

Some work towards #9613.

This removes RepairBuildings and RearmBuildings from Aircraft and Minelayer in favor of using Repairable and the new Rearmable, respectively.

Notes about Rearmable:

  1. The idea is to keep it this simple for the forseeable future (I'd like to keep the Trait(Info)OrDefault approach until the follow-up).
  2. My plan is to later split Repairable into a similarly simple trait (probably keeping the trait name) as well as a generalistic SupplyManager that handles all the orders etc. for both resupply types, and just uses the existence of those traits and - if present - Rearm/RepairActors to decide on the details of the order handling and activity queueing. This would also allow to later introduce other resupply types (Refuel comes to mind).

The yaml changes were done via the provided upgrade rules, so they've been tested and work.

@reaperrr reaperrr added this to the Next + 1 milestone Nov 14, 2017

Show resolved Hide resolved OpenRA.Mods.Cnc/Traits/Minelayer.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/AI/HackyAI.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Activities/Air/HeliReturnToBase.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Activities/Air/HeliReturnToBase.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from c307e99 to 0934e41 Nov 14, 2017

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 14, 2017

Updated.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 0934e41 to 5ded40a Nov 14, 2017

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch 2 times, most recently from 1082dcd to e025e1b Dec 1, 2017

@reaperrr reaperrr removed the PR: Rebase me! label Dec 1, 2017

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 1, 2017

Rebased.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from e025e1b to 95c934c Dec 13, 2017

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 13, 2017

Updated and rebased.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Dec 22, 2017

Needs rebase.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 95c934c to d0525b8 Dec 28, 2017

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 28, 2017

Rebased.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from d0525b8 to 94a0f71 Dec 28, 2017

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 28, 2017

Needs another rebase now.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 94a0f71 to 2a32a3c Dec 29, 2017

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 3bd4c09 to ed69ae8 Oct 7, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 7, 2018

Rebased & updated.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch 2 times, most recently from 0e79ac2 to 58180b0 Oct 7, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 7, 2018

Updated.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 58180b0 to 4a099c1 Oct 14, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 19, 2018

Note that this also blocks progress of my aircraft activity unification roundmap (because I based related branches on this PR to avoid having to make major, regression-risking rebases later).

@pchote
Copy link
Member

pchote left a comment

Unfortunately i've completely lost an overview of this PR, so had to start fresh. Two things stick out as not making sense. I'm afraid that these may have been caused by my earlier scope-creeping requests...

Show resolved Hide resolved OpenRA.Mods.Common/AI/States/AirStates.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Common/Activities/Air/ReturnToBase.cs

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch 3 times, most recently from a4b08c4 to a417ab3 Oct 26, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 26, 2018

Updated (and rebased).

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from a417ab3 to 9201a87 Oct 26, 2018

@pchote
Copy link
Member

pchote left a comment

Code changes LGTM, and I didn't spot anything obviously bogus with a quick test ingame.

This is a step in the right direction, so lets get this merged and any hope any regressions get caught during playtesting.

Needs a rebase, though.

reaperrr added some commits Nov 13, 2017

Remove RepairBuildings from Aircraft
Require them to use Repairable trait instead.
Remove RearmBuildings from Aircraft and Minelayer
In favor of using Rearmable trait.

@reaperrr reaperrr force-pushed the reaperrr:refactor-rearming-pt1 branch from 9201a87 to c11a9de Oct 28, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Oct 28, 2018

Rebased.

@pchote

pchote approved these changes Oct 28, 2018

@pchote pchote merged commit 8f1d8a6 into OpenRA:bleed Nov 2, 2018

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