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 RequiresForceFire to Demolition #15702

Merged
merged 1 commit into from Nov 10, 2018

Conversation

Projects
None yet
4 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Oct 13, 2018

In Sole Survivor, Commando was capable of placing C4 on vehicles, but only when force fired, otherwise it just normally attacked.

Added this bool to be able to replicate this behavior. TESTCASE makes RA Tanya require forcefire to place C4 on buildings (test on enemy buildings, own buildings already require force fire to C4), while can attack normally with no forcefire.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:demolition-force-fire branch from 4bcae21 to 9654c07 Oct 13, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Oct 13, 2018

Updated.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:demolition-force-fire branch from 9654c07 to b86f3a3 Oct 13, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:demolition-force-fire branch from b86f3a3 to e014723 Oct 13, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:demolition-force-fire branch from e014723 to 51c0083 Oct 24, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2018

[16:42:34] | <MustaphaTR> pchote: About #15702 (review), wouldn't make both true would make the code properly work. I think currently it wouldn't normally target an ally building without forcefire even the stances
[16:44:19] | <MustaphaTR> I see some other traits which has Stance checks too and seem to handle the issue like that.

Eww... but ok. This is probably fine to do here, then another PR can handle a proper cleanup.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2018

I filed #15732 to track that cleanup.

@pchote
Copy link
Member

pchote left a comment

LGTM and works ingame. The test case will need to be removed before merging.

@pchote pchote added the PR: Needs +2 label Nov 3, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:demolition-force-fire branch from 51c0083 to 156c33f Nov 4, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Nov 4, 2018

Removed the testcase.

@TheChosenEvilOne
Copy link
Contributor

TheChosenEvilOne left a comment

LGTM and works fine

@abcdefg30 abcdefg30 merged commit 3224843 into OpenRA:bleed Nov 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MustaphaTR MustaphaTR deleted the MustaphaTR:demolition-force-fire branch Nov 10, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Nov 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment