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

Fix MCV Manager glitch when restrict building area is enabled. #17276

Merged
merged 2 commits into from Nov 8, 2019

Conversation

@blackhand1001
Copy link
Contributor

blackhand1001 commented Oct 25, 2019

Fix MCV Manager glitch when restrict building area is enabled. It was checking if the location was close enough to the Base center instead of using the MCV Managers min and max ranges. This would cause it to often have no valid locations despite having a huge range.

Fix MCV Manager glitch when restrict building area is enabled. It was checking if the location was close enough to the Base center instead of using the MCV Managers min and max ranges. This would cause it to often have no valid locations despite having a huge range.
Simplify for loop structure now that it only has one check
@pchote pchote requested a review from reaperrr Oct 25, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 25, 2019

Explanation sounds reasonable, but not tested. It does indeed seem bogus that values of Info.MaxBaseRadius larger than the default placement circle (not to mention building adjacency rules!) used by IsCloseEnoughToBase is ignored.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Oct 26, 2019

You noted on IRC yesterday that

[18:54:12] | <dblaney1> The second mcv behavior seems to be reversed btw. when no mcv exists it looks for somewhere outside the base to deploy
[18:54:21] | <dblaney1> when one exists it places it in the base
[18:55:17] | <dblaney1> i'll have to look at the code. I suspect the if statement is checking for the opposite of what it should

The PR might be unrelated and correct, but given the proximity of time I'd like to note that the behavior described in the comment is - unless I misunderstood something - caused by how restrictToBase works (being false when there is no conyard of the player). This means that the removed IsCloseEnoughToBase only mattered for the secondary replacement MCV. The deploy location for the first MCV is calculated using MaximumTileSearchRange.

Disregard the comment if that was all clear to you.

@blackhand1001

This comment has been minimized.

Copy link
Contributor Author

blackhand1001 commented Oct 26, 2019

Upon further inspection of the code my comments on irc were incorrect. I wrote this pr after making this comment on IRC.

Copy link
Contributor

reaperrr left a comment

I'm pretty sure the usage of IsCloseEnoughToBase here was legacy cruft from the old, monolithic HackyAI.
Fix looks good to me either way.

@abcdefg30 abcdefg30 merged commit d20182f into OpenRA:bleed Nov 8, 2019
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 Nov 8, 2019

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.