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

Fix the close enough check in Repair.cs #13388

Merged
merged 1 commit into from Jun 4, 2017

Conversation

Projects
None yet
3 participants
@abcdefg30
Member

abcdefg30 commented May 29, 2017

Fixes #13384.

The problem is that the helicopter doesn't land at the center position, but is offset.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 29, 2017

Contributor

IMO this is wrong, it works only for offset 256.

Contributor

rob-v commented May 29, 2017

IMO this is wrong, it works only for offset 256.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 May 29, 2017

Member

Updated anyway. This check makes sure that we're on the helipad anyway, so we don't need to close enough check for aircraft.

Member

abcdefg30 commented May 29, 2017

Updated anyway. This check makes sure that we're on the helipad anyway, so we don't need to close enough check for aircraft.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 29, 2017

Contributor

This reverts closeEnough for Aircraft back to WDist.Zero like was before #13358, so it will work.

In that PR we changed it for more actors. As far as I remember, I tested it for vehicle or mine layer (seems not aircraft?), but in case of closeEnough > 0, the IsInRange is still buggy, as it returns false, though actor is in center of target, can't it cause bugs in cases with closeEnough > 0?

Contributor

rob-v commented May 29, 2017

This reverts closeEnough for Aircraft back to WDist.Zero like was before #13358, so it will work.

In that PR we changed it for more actors. As far as I remember, I tested it for vehicle or mine layer (seems not aircraft?), but in case of closeEnough > 0, the IsInRange is still buggy, as it returns false, though actor is in center of target, can't it cause bugs in cases with closeEnough > 0?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 May 29, 2017

Member

Well, it can cause bugs, but I'm not aware of one at the moment. As stated above, the main failure here was the offset when landing the helicopters. Do you have an example for IsInRange failing when the actor is at the CenterPosition and closeEnough > 0 (or where do you think could it fail)?

Member

abcdefg30 commented May 29, 2017

Well, it can cause bugs, but I'm not aware of one at the moment. As stated above, the main failure here was the offset when landing the helicopters. Do you have an example for IsInRange failing when the actor is at the CenterPosition and closeEnough > 0 (or where do you think could it fail)?

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 29, 2017

Contributor

I am fine with this PR.

To your question - it will fail always when closeEnough > 0 so IsInRange checked and actor is e.g. in target's centerposition or close.

Contributor

rob-v commented May 29, 2017

I am fine with this PR.

To your question - it will fail always when closeEnough > 0 so IsInRange checked and actor is e.g. in target's centerposition or close.

@rob-v

rob-v approved these changes May 30, 2017

This reverts the recent closeEnough change for Aircraft so 👍 .

@obrakmann

obrakmann approved these changes Jun 4, 2017 edited

lgtm, 👍

One question, though: should this line in Repairable.cs also be changed? It doesn't affect helicopters, though.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Jun 4, 2017

Member

One question, though: should this line in Repairable.cs also be changed?

I think setting that to WDist.Zero would bring #13357 back. Or do you suggest a different value?

Member

abcdefg30 commented Jun 4, 2017

One question, though: should this line in Repairable.cs also be changed?

I think setting that to WDist.Zero would bring #13357 back. Or do you suggest a different value?

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Jun 4, 2017

Contributor

Right, I just saw how that works. That's alright then.

Contributor

obrakmann commented Jun 4, 2017

Right, I just saw how that works. That's alright then.

@obrakmann obrakmann merged commit 40e8c51 into OpenRA:bleed Jun 4, 2017

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:heliRepair branch Jun 4, 2017

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann
Contributor

obrakmann commented Jun 4, 2017

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