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 OccupiedCells for units sharing cells #17292

Merged
merged 1 commit into from Oct 31, 2019

Conversation

@teinarss
Copy link
Contributor

teinarss commented Oct 29, 2019

Fixes #17290

Disclaimer: The OccuipedCells and crushing logic is wonky as a donkey in the previous release.
I did some testing with adding the Crushable trait to a unit that does not share cells and setting WarnProbability to 100. I observed that the unit had nearly zero chance to dodge a crush successful.

If you log the OccuipedCells during a move you can observe why this is the case (hint: look for when the unit exist in the FromCell)

...
FromCell
FromCell
FromCell
ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell
FromCell
FromCell
...

Unit thats share cells have this pattern when moving to a new cell with at least 2 empty subcells

...
FromCell
FromCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
ToCell
FromCell
FromCell
...

Compare to when there is only 1 free subcell.

...
FromCell
FromCell
FromCell
ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell-ToCell
FromCell
FromCell
FromCell
...

Its probably ok to draw the conclusion that subcell units that trying to dodge a crush by moving to a cell with only 1 free sub cell will fail more often.

Also if the unit need to turn before the move it will fail (depending on how long the turn takes.)

This fix here will hopefully be good enough for fixing the dodging for Infantry.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 29, 2019

Fix confirmed from a quick test, but haven't thought about the consequences or other searched for potential regressions yet.

@pchote pchote added this to the Next Release milestone Oct 29, 2019
@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Oct 29, 2019

This leaves me a bit uncomfortable because it is still an obvious hack. There is no reason to tie this to SharesCell, because the problem is with all crushable units and there is no other reason why infantry should behave different in this regard. That crushable units are also subcell units in the RA mod is purely coincidental and IIRC wasn't even strictly true in the original RA1 either (I believe I recall mammoths could crush light vehicles, but don't quote me on that). The underlying issue is that crushing should not be applied to FromCell in any circumstance.

While this may work for now, it is based on assumptions that can break at any moment, so at the least we should add a comment here that this is a hack and we should try to rethink the way mobile units occupy cells for next +1.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Oct 29, 2019

This leaves me a bit uncomfortable because it is still an obvious hack. There is no reason to tie this to SharesCell, because the problem is with all crushable units and there is no other reason why infantry should behave different in this regard. That crushable units are also subcell units in the RA mod is purely coincidental and IIRC wasn't even strictly true in the original RA1 either (I believe I recall mammoths could crush light vehicles, but don't quote me on that). The underlying issue is that crushing should not be applied to FromCell in any circumstance.

While this may work for now, it is based on assumptions that can break at any moment, so at the least we should add a comment here that this is a hack and we should try to rethink the way mobile units occupy cells for next +1.

Yeah thats the idea, for a proper fix on the whole issue with crushing.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 29, 2019

The idea for prep is to restore the previous behavior in all its known and unknown bogosity - this saves us from having to find and unbreak all the other things (including general gameplay balance) that rely on its specific problems.

@teinarss teinarss force-pushed the teinarss:fix_occupiedcells branch from ea261dd to bd34dcf Oct 30, 2019
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Oct 30, 2019

Added a comment about the hack.

Copy link
Contributor

matjaeck left a comment

I can't find any issues so LGTM.

@pchote pchote added the PR: Needs +2 label Oct 31, 2019
@pchote
pchote approved these changes Oct 31, 2019
Copy link
Member

pchote left a comment

Confirmed the wonky pre-#17159 behaviour, and that this should recreate the previous behaviour except for:

  • Now consistently returns ToCell for infantry instead of only when there are 3 or less units in the cell.
  • Still doesn't return ToCell for a single tick for vehicles.

These edge cases are probably fine to leave out, I can't think of any cases that would rely on them.

@pchote pchote merged commit c94cf61 into OpenRA:bleed Oct 31, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.