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

Change force-fire for artillery-style units into an attack-ground command #16109

Merged
merged 3 commits into from Jan 27, 2019

Conversation

Projects
None yet
5 participants
@pchote
Copy link
Member

pchote commented Jan 25, 2019

Fixes #11663. This PR (which are in the last two commits, after the changes from #16067) implements an ForceFireIgnoresActors flag and enables it for artillery-style units in the default mods.

This feature request has had a lot of attention recently as a way to mitigate some of the less desirably side-effects from the targeting changes.

Depends on #16067 for testing to avoid false-positive issues caused by the wonky bleed fog-targeting behaviour.

@pchote pchote added this to the Next Release milestone Jan 25, 2019

@pchote pchote force-pushed the pchote:artillery-force-fire branch 2 times, most recently from 62298c0 to f326d89 Jan 25, 2019

@matjaeck
Copy link
Contributor

matjaeck left a comment

Edit: Reviewers can skip the next 4 comments, I had misunderstood some things here.


The force fire attack behavior here differs from that in #16112:

Here artillery units that are ordered to force fire on something keep firing when the target is destroyed. I noticed that they will only target the edges of RA oil derricks but not the "+" hitpoints".

In #16112, targeting on an actor with force attack will however target the actor (target flashes) and artillery will stop firing when the target is destroyed (and revealed). The issue with derricks doesn't occur there.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

Yes, that is kind of the point of this pr and the request in #11663 😉

This redefines force-fire for these specific units to attack the terrain in the clicked location, ignoring any actors that may or may not be there.

I can amend the command bar tooltip for the force-attack command to improve the discoverability of this change.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 26, 2019

My point was that this behavior, "attack ground", is changed in #16112 back again. I was not sure if to mention this here or there.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

#16112 isn't based on this one. The two sets of changes will only coexist once both PRs are merged to bleed/prep.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 26, 2019

Thought it is based on this, my bad. Also, the issue with oil derricks was a misperception - artillery target the middle of the cell you clicked just like for any other location. This looks good to me 👍

@pchote pchote force-pushed the pchote:artillery-force-fire branch from f326d89 to af4db94 Jan 26, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

Rebased. This should now be a trivial review.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jan 26, 2019

Would have made more more sense longterm to introduce it as a button on the command bar instead of hacking it into forcefire.

@pchote pchote force-pushed the pchote:artillery-force-fire branch from d381dde to e531ac0 Jan 26, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 26, 2019

That option remains on the table for the future, if there is sufficient need and want for it.

For now, updated the tooltips as discussed in IRC:

@pchote pchote force-pushed the pchote:artillery-force-fire branch from e531ac0 to 3dc3200 Jan 26, 2019

@pchote pchote force-pushed the pchote:artillery-force-fire branch from 3dc3200 to 33b07e1 Jan 27, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 27, 2019

Rebased.

@obrakmann
Copy link
Contributor

obrakmann left a comment

👍

@reaperrr reaperrr merged commit 9c9cad1 into OpenRA:bleed Jan 27, 2019

2 checks passed

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

@pchote pchote deleted the pchote:artillery-force-fire branch Jan 27, 2019

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