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

Allow PortableChrono to queue teleports #16129

Merged
merged 1 commit into from Mar 12, 2019

Conversation

@tovl
Copy link
Contributor

commented Jan 27, 2019

This fixes issue #16106.
It also adresses some concerns expressed in issue #13105.

What this changes:

  • Chronotanks can now be issued queued teleport orders.
  • If ordered to teleport outside of its range, the unit will now first move into teleport range and then teleport.
  • If the unit is not able to teleport (because it is charging) it will fall back to a normal move instead.
  • When ordered to teleport, the unit displays a target line in the same color as the teleport range circle.
  • All of the above works for both ForceMove orders and deploy orders.

This still does not allow the deploy hotkey to be used.

@pchote pchote referenced this pull request Jan 27, 2019

@tovl tovl changed the title allow PortableChrono to queue teleports Allow PortableChrono to queue teleports Feb 10, 2019

@tovl tovl force-pushed the tovl:queue-chrono branch from bfe2a29 to 169d9bd Feb 21, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

Two minor suggestions:

  • since it's implicitly required anyway, I recommend adding Requires<IMoveInfo> to PortableChrono
  • although the performance impact won't be measurable, I suggest caching IMove
@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

Implemented the above improvements.

@tovl tovl force-pushed the tovl:queue-chrono branch from e6de043 to 725f222 Mar 4, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

GitHub doesn't show the merge conflict here, but this also needs to be rebased after #16246.

@tovl tovl force-pushed the tovl:queue-chrono branch from 725f222 to 2664b59 Mar 9, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Rebased.

@pchote pchote removed the PR: Rebase me! label Mar 9, 2019

@pchote
Copy link
Member

left a comment

This seems like a great improvement ingame. Just one code nit, which was actually already raised earlier. LGTM otherwise.

OpenRA.Mods.Cnc/Traits/PortableChrono.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

OpenRA.Mods.Cnc/Traits/PortableChrono.cs:L62: [SA1215] All non-static readonly private fields must be placed before all non-static non-readonly private fields.

make portable chrono queueable
give PortableChrono fallback movement

style fix

add chrono target line

require and cache IMove

@tovl tovl force-pushed the tovl:queue-chrono branch from c068a3d to 71c3c84 Mar 10, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Doh! I only stylechecked the Openra.mods.Common assembly. Forgot that this was an RA specific thing.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

If ordered to teleport outside of its range, the unit will now first move into teleport range and then teleport.

This might be a point of contention. Am currently asking people on Discord what they think. What came to my mind here: When your chrono tanks are in danger you often just want to immediately teleport them away as far as possible which wouldn't be possible with this change.
Another point is: Doesn't queuing a move and then a teleport order achieve the same without enforcing the behaviour?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

If ordered to teleport outside of its range, the unit will now first move into teleport range and then teleport.

What came to my mind here: When your chrono tanks are in danger you often just want to immediately teleport them away as far as possible which wouldn't be possible with this change.

On the other hand, the proposed behavior here ensures that you chrono to the actual target location, which is from my experience more important (and in contrast more frustrating if you accidentally chrono in front of a cliff cell instead of behind the cliff as intended). I can understand if other people think different, it probably depends on playstyle which behavior is desirable.

@abcdefg30
Copy link
Member

left a comment

Consensus on Discord seems to be "let's wait and see". And this is certainly an improvement.

@abcdefg30 abcdefg30 merged commit c096fbd into OpenRA:bleed Mar 12, 2019

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

commented Mar 12, 2019

Changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.