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 "Assault Move" modifier to attack move #14010

Merged
merged 7 commits into from Sep 11, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Sep 10, 2017

As discussed in other tickets, IRC, and on the forum. This follows the lead from C&C3 in providing an a quick and stateless attack-anything + attack move variation.

This also fixes autotargeting vs buildings in TS, and adds an attack-move cursor to d2k (in addition to assault-move cursors for RA and TD). The second commit will need to be edited to work with the older hotkey setup when picking to prep.

Closes #14008

@pchote pchote added this to the Next Release milestone Sep 10, 2017

@pchote pchote requested a review from obrakmann Sep 10, 2017

{
keyOverrides.OnKeyPress = ki =>
{
// HACK: enable attack move to be triggered if the ctrl key is pressed

This comment has been minimized.

@pchote

pchote Sep 10, 2017

Member

Note: this will be fixed by the next phase of my hotkey rework, but that is currently blocked on #13781, #13966, #13974 and in any case won't help on the prep branch.

@pchote

pchote Sep 10, 2017

Member

Note: this will be fixed by the next phase of my hotkey rework, but that is currently blocked on #13781, #13966, #13974 and in any case won't help on the prep branch.

@obrakmann

Looks good and works as expected.

I didn't see any new cursor in TS, is that intentional?

Show outdated Hide outdated OpenRA.Mods.Common/Traits/AttackMove.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

I didn't see any new cursor in TS, is that intentional?

Yeah, we don't have any attack move cursors in TS yet, and I don't have a functional indexed-image editor to make them. @reaperrr graciously did ra/cnc/d2k cursors here, and I didn't want to push my luck asking for another two for ts.

Member

pchote commented Sep 10, 2017

I didn't see any new cursor in TS, is that intentional?

Yeah, we don't have any attack move cursors in TS yet, and I don't have a functional indexed-image editor to make them. @reaperrr graciously did ra/cnc/d2k cursors here, and I didn't want to push my luck asking for another two for ts.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Fixed.

Member

pchote commented Sep 10, 2017

Fixed.

@obrakmann

👍

The way this works doesn't stand in the way of the attack-move rewrite, either. The condition granting/revoking can then eventually be moved from AttackMove.Tick to AttackMoveActivity.On(First|Last)Run.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Yup, that was my plan/hope. The conditions should then also hopefully stay active all the time, instead of being dropped when the actor is attacking, which will make them useful for more than just toggling target types.

Member

pchote commented Sep 10, 2017

Yup, that was my plan/hope. The conditions should then also hopefully stay active all the time, instead of being dropped when the actor is attacking, which will make them useful for more than just toggling target types.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Added cursors for TS again thanks to @reaperrr.

Member

pchote commented Sep 10, 2017

Added cursors for TS again thanks to @reaperrr.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2017

Contributor

Looks good to me and since only the TS cursors changed I consider @obrakmann's +1 still valid, but before I merge this: @pchote would you mind doing the picking once merged, since I'm not sure how much the 2nd commit needs to be edited for prep? I don't want to mess that up.

Contributor

reaperrr commented Sep 10, 2017

Looks good to me and since only the TS cursors changed I consider @obrakmann's +1 still valid, but before I merge this: @pchote would you mind doing the picking once merged, since I'm not sure how much the 2nd commit needs to be edited for prep? I don't want to mess that up.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

Yup, can do, although it will need to be tomorrow.

Member

pchote commented Sep 10, 2017

Yup, can do, although it will need to be tomorrow.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 11, 2017

Member

I pushed an assault-move-playtest branch that is based on prep-1707. You can cherry pick safely from that when merging.

Member

pchote commented Sep 11, 2017

I pushed an assault-move-playtest branch that is based on prep-1707. You can cherry pick safely from that when merging.

@reaperrr reaperrr merged commit 049ed08 into OpenRA:bleed Sep 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
if (world.Map.Contains(cell))
return mi.Modifiers.HasModifier(Modifiers.Ctrl) ? "assaultmove" : "attackmove";
return "generic-blocked";

This comment has been minimized.

@Mailaender

Mailaender Sep 16, 2017

Member

Why generic-blocked instead of assaultmove-blocked?

@Mailaender

Mailaender Sep 16, 2017

Member

Why generic-blocked instead of assaultmove-blocked?

This comment has been minimized.

@pchote

pchote Sep 16, 2017

Member

This was maintaining the existing behaviour - we didn't have any assaultmove-blocked cursors at the time.

@pchote

pchote Sep 16, 2017

Member

This was maintaining the existing behaviour - we didn't have any assaultmove-blocked cursors at the time.

@pchote pchote deleted the pchote:assault-move branch Oct 15, 2017

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