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 OccupiesSpace check to production cycling #17216

Merged
merged 1 commit into from Feb 26, 2020

Conversation

@abc013
Copy link
Contributor

abc013 commented Oct 11, 2019

Closes #16703.

The first commit removes the BaseBuilding requirement for production cycling, since e.g. mobile actors without that trait can be producers as well. Testcase: Add any production to any mobile actor.

The second commit prevents cycling to an actor that does not occupy space, e.g. the player actor.

@reaperrr reaperrr mentioned this pull request Nov 17, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 22, 2020

The purpose of the BaseBuilding check is actually to stop construction yards from being included in the selection. It doesn't have anything to do with Buildings!

IMO adding construction yards to the rotation is a regression because they don't have any rallypoints to adjust, and have their own dedicated selection hotkey.

@abc013 abc013 force-pushed the abc013:cycleProductionFix branch from d809e07 to 219c53f Feb 22, 2020
@abc013

This comment has been minimized.

Copy link
Contributor Author

abc013 commented Feb 22, 2020

I'm thinking about another solution for the first commit, but this may take a while.
Removed the first commit.

@abc013 abc013 changed the title Remove BaseBuilding prerequisite and add OccupiesSpace check to production cycling Add OccupiesSpace check to production cycling Feb 22, 2020
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Feb 22, 2020

This isn't quite enough to fix the crash on bomber john: I suspect that the if (next == null) next = facilities.First(); check just below will be setting it back to the player.

Edit: The issue is that the latest bleed has #17490 which introduces a second implementation of the same bug. Would you be able to rebase and fix that one too?

@abc013 abc013 force-pushed the abc013:cycleProductionFix branch from 219c53f to 0f4cd1f Feb 22, 2020
@abc013

This comment has been minimized.

Copy link
Contributor Author

abc013 commented Feb 22, 2020

Oops, sorry. Updated.

@abc013 abc013 force-pushed the abc013:cycleProductionFix branch from 0f4cd1f to df9f098 Feb 23, 2020
@pchote
pchote approved these changes Feb 23, 2020
Copy link
Member

pchote left a comment

LGTM and we should take this crash fix for the playtest.

@pchote pchote added this to the Next Release milestone Feb 23, 2020
@pchote pchote added the PR: Needs +2 label Feb 23, 2020
@abcdefg30 abcdefg30 merged commit c0ece00 into OpenRA:bleed Feb 26, 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 26, 2020

@abc013 abc013 deleted the abc013:cycleProductionFix branch Feb 27, 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.

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