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 lm cache movement #17159

Merged
merged 2 commits into from Oct 5, 2019

Conversation

@teinarss
Copy link
Contributor

teinarss commented Sep 29, 2019

Fixes #17089

@pchote pchote added this to the Next Release milestone Sep 29, 2019
@@ -214,8 +214,6 @@ public int Facing
{
if (FromCell == ToCell)
return new[] { Pair.New(FromCell, FromSubCell) };
if (CanEnterCell(ToCell))
return new[] { Pair.New(ToCell, ToSubCell) };

return new[] { Pair.New(FromCell, FromSubCell), Pair.New(ToCell, ToSubCell) };

This comment has been minimized.

Copy link
@tovl

tovl Sep 29, 2019

Contributor

Shouldn't this be return new[] { Pair.New(ToCell, ToSubCell) }; now if we want the unit behaviour to remain the same?

This comment has been minimized.

Copy link
@pchote

pchote Sep 29, 2019

Member

the idea behind the old code was that units occupied both the cell they were leaving and the cell they were entering while moving. This avoids units stacking on top of eachother if the movement is stopped (e.g. EMP) before it has left the starting cell.

This comment has been minimized.

Copy link
@tovl

tovl Sep 30, 2019

Contributor

It is obvious that that was the idea. I'm just pointing out that that is not how units have been behaving all this time—probably never have—because CanEnterCell(ToCell) is always true. It only gets set after checking whether the next cell in the path is accessible and then the actor immediately lays claim to it so no other actor can block it anymore.

The idea was also probably a bad one to begin with. It has no effect on the case where units stop before it has left the starting cell, because such a window doesn't exist in the first place. The new mobile.ToCell is set in the same tick as the MovePart is queued (which is not interruptible and always the first childactivity when it is queued) so an EMPed unit will always end up in ToCell no matter when the movement was stopped. If it hasn't started moving yet, ToCell is equal to FromCell.

What this will affect is movement of groups of units. Because units in a column will have to wait till the unit in front of it has completely finished moving to the next cell before starting its move, it means tight formations are no longer possible. There will be odd gaps in the formation and starting a movement will be slow and chaotic.

In any case, this doesn't seem like the kind of behavioural change you would want to do this short before a release as part of a simple bugfix that is not, on the face of it, really related. And since it is just half a line difference to keep the status quo behaviour (intentional or not) while still fixing the bug, it seems prudent to do so.

This comment has been minimized.

Copy link
@abcdefg30

abcdefg30 Sep 30, 2019

Member

Since we don't allow changing subcells while remaining in the same cell, (FromCell == ToCell) => (FromSubCell == ToSubCell) right? In that case this entire method can simplify down to return new[] { Pair.New(ToCell, ToSubCell) };.

This comment has been minimized.

Copy link
@teinarss

teinarss Sep 30, 2019

Author Contributor

Since we don't allow changing subcells while remaining in the same cell, (FromCell == ToCell) => (FromSubCell == ToSubCell) right? In that case this entire method can simplify down to return new[] { Pair.New(ToCell, ToSubCell) };.

Looks like it.

This comment has been minimized.

Copy link
@pchote

pchote Sep 30, 2019

Member

That means that CanEnterCell(ToCell) is not always true

Correct. This has always had the definition quirk of “I can’t enter if I’m already in it”, and things break when this is changed. IIRC CanExistInCell was added to handle the “already in or can move into cell” case.

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 1, 2019

Author Contributor

The transition of OccupiedCells when moving between two cells looks like this on the previous release (20190314). Imo it was best to look at it before we added the locomotor cache in case of any regressions.

FromCell
ToCell
FromCell-ToCell
FromCell

It only spends a short time on ToCell and the majority on FromCell-ToCell while the move happens. I.e. in only got one line of ToCell logged.

This comment has been minimized.

Copy link
@pchote

pchote Oct 1, 2019

Member

Can you update the transition list to note where the values of fromcell and tocell are the same or different?

This comment has been minimized.

Copy link
@teinarss

teinarss Oct 1, 2019

Author Contributor

The only time the fromcell and tocell are the same is when FromCell is printed.

This is how the code looks:

		public Pair<CPos, SubCell>[] OccupiedCells()
		{
			if (FromCell == ToCell)
			{
				Debug.WriteLine("FromCell");
				return new[] { Pair.New(FromCell, FromSubCell) };
			}

			if (CanEnterCell(ToCell))
			{
				Debug.WriteLine("ToCell");
				return new[] { Pair.New(ToCell, ToSubCell) };
			}

			Debug.WriteLine("FromCell-ToCell");
			return new[] { Pair.New(FromCell, FromSubCell), Pair.New(ToCell, ToSubCell) };
		}

Complete log for a move, only one actor on the map and dev visibility (for enabling radar).

...
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
...

This comment has been minimized.

Copy link
@pchote

pchote Oct 28, 2019

Member

Are we aware of all the possible downstream affects from changing the fundamental definition of what cells a mobile unit occupies to consuming code? It feels like asking for regressions.

#17290

@teinarss teinarss force-pushed the teinarss:fix_lm_cache_movement branch from 8cb211e to 435d3db Sep 30, 2019
@teinarss teinarss marked this pull request as ready for review Sep 30, 2019
@teinarss teinarss force-pushed the teinarss:fix_lm_cache_movement branch from 435d3db to 6e02d13 Oct 1, 2019
@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Oct 2, 2019

FWIW I have tested this in RA and TS without experiencing any de-sync crashes.

Copy link
Member

pchote left a comment

The code changes here now look sensible, so we are probably best with merging this and letting it be tested by a wider audience.

The if (CanEnterCell(ToCell)) branch this removes has been present since the very early days, and doesn't make a lot of sense - actors should either be in a single cell (when ToCell == FromCell) or spanning both cells - I can't think of a logically valid case where we would want to return only the ToCell when FromCell is different. Removing this may actually fix some hidden bugs.

@@ -270,6 +270,7 @@ public interface IActorMap
WDist LargestActorRadius { get; }
WDist LargestBlockingActorRadius { get; }

void MarkAsDirty(IOccupySpace ios);

This comment has been minimized.

Copy link
@pchote

pchote Oct 4, 2019

Member

MarkAsDirty isn't very descriptive. How about something like (Dirty|Update|Refresh)OccupiedCells?

OpenRA.Mods.Common/Traits/World/Locomotor.cs Outdated Show resolved Hide resolved
@teinarss teinarss force-pushed the teinarss:fix_lm_cache_movement branch from 6e02d13 to 03f5e5c Oct 5, 2019
@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented Oct 5, 2019

updated

@pchote
pchote approved these changes Oct 5, 2019
Copy link
Member

abcdefg30 left a comment

Changes look good and I didn't experience any desyncs or other crashes.

@abcdefg30 abcdefg30 merged commit d34bce9 into OpenRA:bleed Oct 5, 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 Oct 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.