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

AttackFrontal's FacingTolerance is now in effect #13530

Merged
merged 1 commit into from Aug 7, 2017

Conversation

Projects
None yet
4 participants
@forcecore
Contributor

forcecore commented Jun 20, 2017

FacingTolerance didn't work as expected. Add new facing check code to make it work.

  • Allows modders to make cool units that don't need to turn when attacking: https://www.youtube.com/watch?v=kgPlmc85i6Y

  • There's a catch though. If the unit is facing North (facing 0) and ordered to attack NorthEast (Facing =~ 230) it will turn due to "wrong" condition, as their absolute difference is big. I copy pasted this condition from AttackFrontal.cs:CanAttack() facing check code so if this seems wrong that should be fixed as well.

  • I'm wondering why the default value for FacingTolerance is 1, when it is effectively 0. Unless there's a good reason I wish to make it 0 and make the facing check condition simpler. (rather than <= 1, I want to make it != 0.)

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jun 20, 2017

Contributor

Tolerance check corrected

Contributor

forcecore commented Jun 20, 2017

Tolerance check corrected

@GraionDilach

👍 after squash.

public override void RulesetLoaded(Ruleset rules, ActorInfo ai)
{
if (FacingTolerance < 0 || FacingTolerance > 128)
throw new YamlException("Facing tolerance must be in range of [0, 128], 128 covers 360 degrees.");

This comment has been minimized.

@pchote

pchote Jul 15, 2017

Member

There are 256 facing units to a full circle, so 128 is only 180 degrees?

@pchote

pchote Jul 15, 2017

Member

There are 256 facing units to a full circle, so 128 is only 180 degrees?

This comment has been minimized.

@GraionDilach

GraionDilach Jul 15, 2017

Contributor

I believe all cases (including this PR) applies Tolerance to both sides, so that a value of 1 allows 1 facing unit be tolerated to the left and 1 facing unit tolerated to the right.

@GraionDilach

GraionDilach Jul 15, 2017

Contributor

I believe all cases (including this PR) applies Tolerance to both sides, so that a value of 1 allows 1 facing unit be tolerated to the left and 1 facing unit tolerated to the right.

This comment has been minimized.

@forcecore

forcecore Jul 15, 2017

Contributor

Graion is right. To cover 180 degrees you need to give 64: +-90 degrees.

@forcecore

forcecore Jul 15, 2017

Contributor

Graion is right. To cover 180 degrees you need to give 64: +-90 degrees.

@pchote

I haven't tested this yet, but the changes look simple and correct. I just have a couple of minor code style requests:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Attack/AttackFrontal.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Attack/AttackFrontal.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Attack/AttackFrontal.cs Outdated
@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jul 26, 2017

Contributor

Squashed and FacingWithinTolerance is now in Util.cs, along with other facing functions.

Contributor

forcecore commented Jul 26, 2017

Squashed and FacingWithinTolerance is now in Util.cs, along with other facing functions.

@abcdefg30 abcdefg30 merged commit d132821 into OpenRA:bleed Aug 7, 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 Aug 7, 2017

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