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

Fixed AttackMove ignoring MoveIntoShroud settings #13935

Merged
merged 3 commits into from Oct 16, 2017

Conversation

Projects
None yet
5 participants
@Mailaender
Member

Mailaender commented Aug 29, 2017

Closes #10016.

@SoScared

SoScared approved these changes Sep 2, 2017 edited

TS' air scout mechanics looks a bit funny in this day and age. Regardless, I can't speak for the design of code itself but the fix works as promised. All air units are unable to enter shroud by right-clicking on it.

@pchote

This shows the regular attack move cursor over the shroud, and the units then don't respond when ordered. The cursor needs to change to a -blocked variant for this to not feel like a bug.

  • TS doesn't have an attackmove-blocked cursor (it doesn't have a normal attackmove cursor either), so you can reuse move-blocked as a placeholder.
  • GenericSelectTarget doesn't have the plumbing to understand this, so you will need to implement a proper order generator for this.
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 9, 2017

Member

I implemented a custom AttackMoveOrderGenerator as part of my proposed fix for #14001. If that turns into a PR then you'll be able to rebase and apply the cursor fixes on top of that, and if it doesn't then you can still adopt the code here directly.

Member

pchote commented Sep 9, 2017

I implemented a custom AttackMoveOrderGenerator as part of my proposed fix for #14001. If that turns into a PR then you'll be able to rebase and apply the cursor fixes on top of that, and if it doesn't then you can still adopt the code here directly.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 15, 2017

Contributor

#14010 has been merged, so this can be rebased now.

Contributor

reaperrr commented Sep 15, 2017

#14010 has been merged, so this can be rebased now.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 16, 2017

Member

Updated to also show the correct cursor.

Member

Mailaender commented Sep 16, 2017

Updated to also show the correct cursor.

@pchote

The changes here look good to me, but while testing this I found a couple of issues with the underlying AttackMoveOrderGenerator. I will file a PR to fix those first to save on merge conflicts when picking to prep.

@pchote pchote dismissed their stale review Sep 24, 2017

Code updated.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote
Member

pchote commented Sep 24, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 28, 2017

Contributor

#14070 is merged, needs rebase.

Contributor

reaperrr commented Sep 28, 2017

#14070 is merged, needs rebase.

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Sep 30, 2017

Member

Rebased.

Member

Mailaender commented Sep 30, 2017

Rebased.

@pchote

pchote approved these changes Oct 14, 2017

@pchote pchote added the PR: Needs +2 label Oct 14, 2017

@abcdefg30 abcdefg30 merged commit ebb9827 into OpenRA:bleed Oct 16, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Oct 16, 2017

@Mailaender Mailaender deleted the Mailaender:attackmove-intoshroud branch Oct 16, 2017

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