Skip to content
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 TransformsIntoTransforms to enable queued MCV redeploy. #17615

Merged
merged 2 commits into from Feb 24, 2020

Conversation

@pchote
Copy link
Member

pchote commented Jan 25, 2020

Fixes #17602.

@pchote pchote added this to the Next+1 milestone Jan 25, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 12, 2020

This has been sitting open on the milestone with no activity for quite a while now. Can we please get some movement here, even if that happens to be an "I don't think we should take this for the playtest"?

@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Feb 15, 2020

Sorry, I can't test this as the ALT key is somehow blocked in KDE even in full screen mode and the conyard won't undeploy at all. See #15879 for making it a remappable key.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 15, 2020

Copy link
Member

Mailaender left a comment

Works as promised.

@pchote pchote force-pushed the pchote:transforms-into-transforms branch 2 times, most recently from bc3c661 to 3ac1cdf Feb 18, 2020
@pchote pchote force-pushed the pchote:transforms-into-transforms branch from 3ac1cdf to 33e6d1a Feb 18, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 18, 2020

Fixed (first and third pushes) and rebased (second push).

@pchote pchote requested a review from teinarss Feb 18, 2020
Copy link
Member

abcdefg30 left a comment

Works as advertised. One thing however: When undeploying you can now issue deploy orders. However, when deploying you can also issue deploy orders (to immediately undeploy again) which are not executed. Not supporting immediate undeploy would not be an issue imho if the command bar icon for deploying was disabled.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 20, 2020

Hmm... this may require more thought then. I had always considered blocking immediate undeploy as a protection against accidental double keypresses so its not clear whether we want to support that. Keeping the current assymetric state of the PR also isn't great.

Would it be confusing if we only allowed this queued redeploy if the actor has moved to a different location?

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 20, 2020

However, when deploying you can also issue deploy orders (to immediately undeploy again) which are not executed.

Are you sure about this? https://github.com/OpenRA/OpenRA/pull/17615/files#diff-77ec43bbb0b6febb3e3e53220fef3002R37-R42 is supposed to make sure that you can only issue deploy orders when undeploying - the initial deploy animation should be handled by the make animation rather than Transforms.

@WhoCaresora

This comment has been minimized.

Copy link

WhoCaresora commented Feb 20, 2020

I don't see a single case where queuing order when deploying is usefull as the only thing a conyard can do is undeploy.

The reasons that come in mind to deploy and mcv :

  • To have build radius and by extension placing a building. -- Queuing immediate undeploy would not give the necessary time to place a single structure
  • To have additional construction speed -- You want your mcv as conyard form until it is needed somewhere so again queuing a undeploying order does not apply
  • changing mcv armor into conyard concrete -- You might want your mcv in conyard until the zone is safe so undeploying immediatly put you back in a vulnerable position.
  • Escaping a hijacker. I guess with hijacker you could deploy-undeploy to make his targeting void as he can't target building and hope your opponent to not be attentive (but seriously who would watch somewhere else when he is about to capture and mcv).
    -escaping any unit with opportunity fire. Unit with opportunity fire do not transfer the focus fire over transformation. you can make planes "bug" big time and you can flee from tanks.
@Punsho

This comment has been minimized.

Copy link
Contributor

Punsho commented Feb 20, 2020

Hmm... this may require more thought then. I had always considered blocking immediate undeploy as a protection against accidental double keypresses so its not clear whether we want to support that. Keeping the current assymetric state of the PR also isn't great.

Would it be confusing if we only allowed this queued redeploy if the actor has moved to a different location?

A normal deploy order while deploying should definitely not queue an undeploy. If it was a case it would become a source of frustration to many people. However allowing shift-f to queue an undeploy should be done for the sake of consistency and player expectations. Though it is not necessary to do now as there are no ingame incentives for this action apart from providing temporary construction radius (what would be clunky and rarely the desired action)

@pchote pchote force-pushed the pchote:transforms-into-transforms branch from 33e6d1a to 912dc7a Feb 20, 2020
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 20, 2020

Fixed. The commandbar button now stays disabled unless shift is held to queue the redeploy.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 23, 2020

I count two 👍s here. @abcdefg30 did you want to take another look here or shall I merge this?

@abcdefg30 abcdefg30 merged commit 6ba0280 into OpenRA:bleed Feb 24, 2020
2 checks passed
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.

Copy link
Member

abcdefg30 commented Feb 24, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.