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 Service Depot Rally point path finding #13093

Merged
merged 1 commit into from May 7, 2017

Conversation

Projects
None yet
3 participants
@rob-v
Contributor

rob-v commented Apr 9, 2017

Fix -Repaired unit moved towards Rally point but stopped on first obstacle instead of finding path like with Move order or with Production Rally Point.

Fixes #3757, #12965

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 9, 2017

Contributor

Ok, thanks. silly mistake, I missed the fact that movement.MoveTo returns new Move so I just need to use the right one. I use the same as with Move order now and it works properly.

Contributor

rob-v commented Apr 9, 2017

Ok, thanks. silly mistake, I missed the fact that movement.MoveTo returns new Move so I just need to use the right one. I use the same as with Move order now and it works properly.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 10, 2017

Member

I don't think this is the best fix here - it looks like the real bug is inside IMove.MoveTo(CPos, Actor), and while this might technically fix the service depot rally move, it doesn't resolve the real issue.

Member

pchote commented Apr 10, 2017

I don't think this is the best fix here - it looks like the real bug is inside IMove.MoveTo(CPos, Actor), and while this might technically fix the service depot rally move, it doesn't resolve the real issue.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 10, 2017

Contributor

That differently working Move ctor was suspicious to me, but as I don't have enough experience with pathing I didn't investigate more if that is intended behaviour or not. It looked to me also reasonable that move to rally point from SD works like Move order. I don't know if that ctor is buggy or correct but shouldn't be used here. FindPath(search) (not working) vs .FindUnitPath(mobile.ToCell, destination, self) (working). I will try to investigate if you don't agree with current fix.

Contributor

rob-v commented Apr 10, 2017

That differently working Move ctor was suspicious to me, but as I don't have enough experience with pathing I didn't investigate more if that is intended behaviour or not. It looked to me also reasonable that move to rally point from SD works like Move order. I don't know if that ctor is buggy or correct but shouldn't be used here. FindPath(search) (not working) vs .FindUnitPath(mobile.ToCell, destination, self) (working). I will try to investigate if you don't agree with current fix.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 10, 2017

Member

This is caused by Move.cs#L88 running the pathfinder with checkForBlocked = false. This makes it ignore all actors, not just the one we want.

Member

pchote commented Apr 10, 2017

This is caused by Move.cs#L88 running the pathfinder with checkForBlocked = false. This makes it ignore all actors, not just the one we want.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 10, 2017

Contributor

I don't know what is the right solution, if checkForBlocked = false is sometimes valid or we should change it to true (called from LayMines.Tick and twice from Repairable.AfterReachActivities).

Contributor

rob-v commented Apr 10, 2017

I don't know what is the right solution, if checkForBlocked = false is sometimes valid or we should change it to true (called from LayMines.Tick and twice from Repairable.AfterReachActivities).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 10, 2017

Member

It is valid for the nudge and scripted moves that are currently called via the Move(Actor, CPos) ctor (by definition/design they are supposed to stop if blocked), but should never be valid for the general moves that call Move(Actor, CPos, Actor), which we don't want to stop if blocked.

Member

pchote commented Apr 10, 2017

It is valid for the nudge and scripted moves that are currently called via the Move(Actor, CPos) ctor (by definition/design they are supposed to stop if blocked), but should never be valid for the general moves that call Move(Actor, CPos, Actor), which we don't want to stop if blocked.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Apr 10, 2017

Contributor

Thanks, updated. The fix now looks trivial.

Contributor

rob-v commented Apr 10, 2017

Thanks, updated. The fix now looks trivial.

@rob-v rob-v referenced this pull request Apr 20, 2017

Closed

Repair pad pathfinding #3757

@pchote pchote added the PR: Needs +2 label Apr 30, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

As far as I can tell, this actually makes it worse.
If the rally point is blocked, the repaired actor now won't even leave the pad.

Contributor

reaperrr commented May 1, 2017

As far as I can tell, this actually makes it worse.
If the rally point is blocked, the repaired actor now won't even leave the pad.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 1, 2017

Contributor

You're right. Before this PR, it was blocked on a obstacle, but in this PR it doesn't leave SP in case of blocked rally point, though it finds a path otherwise.

Initially my PR resolved this issue by changing it to use the same Move(Actor self, CPos destination, WDist nearEnough) order used by normal Move order or move to RallyPoint of built units. We changed it to keep the current ctor but 'fix' one parameter.
I don't know what is the correct action - change it to use the same Move (which finds a path, isn't blocked by blocked rally point) or ...

Contributor

rob-v commented May 1, 2017

You're right. Before this PR, it was blocked on a obstacle, but in this PR it doesn't leave SP in case of blocked rally point, though it finds a path otherwise.

Initially my PR resolved this issue by changing it to use the same Move(Actor self, CPos destination, WDist nearEnough) order used by normal Move order or move to RallyPoint of built units. We changed it to keep the current ctor but 'fix' one parameter.
I don't know what is the correct action - change it to use the same Move (which finds a path, isn't blocked by blocked rally point) or ...

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 1, 2017

Member

Move(Actor self, CPos destination, Actor ignoredActor) should change to use the same kind of path searching as the others, but keep the WithIgnoredActor modifier.

Member

pchote commented May 1, 2017

Move(Actor self, CPos destination, Actor ignoredActor) should change to use the same kind of path searching as the others, but keep the WithIgnoredActor modifier.

@rob-v rob-v changed the title from Service pad - rep.unit moves to Rally point instead of stopping on first obstacle #12965 to Fix Service Depot Rally point path finding May 1, 2017

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 1, 2017

Contributor

Updated.

Contributor

rob-v commented May 1, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 4, 2017

Contributor

Looks good to me now. If there's an obstacle in the middle of the path, the actor moves around it. If the rallypoint is blocked by a mobile actor, it nudges the blocker away. If the RP is blocked by an immobile actor (building), it moves as close as possible.
Code looks good, too, as far as I can tell, but I'd like @pchote to reconfirm his 👍 before this is merged, since he approved this before the code was updated.

Contributor

reaperrr commented May 4, 2017

Looks good to me now. If there's an obstacle in the middle of the path, the actor moves around it. If the rallypoint is blocked by a mobile actor, it nudges the blocker away. If the RP is blocked by an immobile actor (building), it moves as close as possible.
Code looks good, too, as far as I can tell, but I'd like @pchote to reconfirm his 👍 before this is merged, since he approved this before the code was updated.

@pchote pchote dismissed their stale review May 4, 2017

Code has been rewritten.

@pchote

Yup, looks good. I have two minor style nits, then 👍

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 6, 2017

Contributor

Updated.

Contributor

rob-v commented May 6, 2017

Updated.

@pchote

pchote approved these changes May 7, 2017

@pchote pchote merged commit e6ec071 into OpenRA:bleed May 7, 2017

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