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

Move activity now uses ChildActivity #13749

Merged
merged 1 commit into from Nov 12, 2017

Conversation

Projects
None yet
5 participants
@forcecore
Contributor

forcecore commented Jul 29, 2017

Removing THE obstacle for waypoint visualization PR #13336.

In line 245, the tick runs the child activity before returning. I commented why. However, what I didn't write there is that it changes the behavior when the move order has first ticked to that part. It used to return MoveFirstHalf and not tick it.

I also tried to make Move activity more understandable but, I failed because I couldn't understand Move.cs. I am not even 50% sure what MoveFirstHalf and MoveSecondHalf are. No comments and not self-explanatory :( I'll just make minimal changes.

Show outdated Hide outdated OpenRA.Mods.Common/Activities/Move/Move.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Activities/Move/Move.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Activities/Move/Move.cs Outdated

@pchote pchote requested a review from obrakmann Jul 29, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 29, 2017

Member

I am not even 50% sure what MoveFirstHalf and MoveSecondHalf are. No comments and not self-explanatory :( I'll just make minimal changes.

MoveFirstHalf moves the actor from its start point (either its starting subcell or the edge of a cell for chained moves) to the edge of the next cell. MoveSecondHalf moves the actor from the edge of the cell back to a defined subcell.

Member

pchote commented Jul 29, 2017

I am not even 50% sure what MoveFirstHalf and MoveSecondHalf are. No comments and not self-explanatory :( I'll just make minimal changes.

MoveFirstHalf moves the actor from its start point (either its starting subcell or the edge of a cell for chained moves) to the edge of the next cell. MoveSecondHalf moves the actor from the edge of the cell back to a defined subcell.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Jul 29, 2017

Contributor

MoveFirstHalf moves the actor from its start point (either its starting subcell or the edge of a cell for chained moves) to the edge of the next cell. MoveSecondHalf moves the actor from the edge of the cell back to a defined subcell.

Suggestion: rename them MoveOutOfCell and MoveIntoCell, respectively, to make their purpose clearer?

Contributor

obrakmann commented Jul 29, 2017

MoveFirstHalf moves the actor from its start point (either its starting subcell or the edge of a cell for chained moves) to the edge of the next cell. MoveSecondHalf moves the actor from the edge of the cell back to a defined subcell.

Suggestion: rename them MoveOutOfCell and MoveIntoCell, respectively, to make their purpose clearer?

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jul 29, 2017

Contributor

Suggestion: rename them MoveOutOfCell and MoveIntoCell, respectively, to make their purpose clearer?

I also tried to modify moveFraction to moveStep, TotalMoveFraction to TotalMoveSteps, as they have nothing to do with division. Then I changed my mind. I want to induce fewer changes so that the correctness is easier to be seen. I think it would be better to make rename PR right after this one.

Contributor

forcecore commented Jul 29, 2017

Suggestion: rename them MoveOutOfCell and MoveIntoCell, respectively, to make their purpose clearer?

I also tried to modify moveFraction to moveStep, TotalMoveFraction to TotalMoveSteps, as they have nothing to do with division. Then I changed my mind. I want to induce fewer changes so that the correctness is easier to be seen. I think it would be better to make rename PR right after this one.

@obrakmann

Looks good so far, just one minor nit. Turning on the StrictActivityChecking setting doesn't crash in Move anymore, so that's a good. I'd like to do some more in-game testing, though.

Show outdated Hide outdated OpenRA.Mods.Common/Activities/Move/Move.cs Outdated
@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jul 30, 2017

Contributor

Minor code correction reflecting comments above.

Contributor

forcecore commented Jul 30, 2017

Minor code correction reflecting comments above.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 17, 2017

Member

My understanding is that @obrakmann has made some progress here, but is currently stalled on an unknown but possibly unrelated crash issue:

From 2017-08-13 (#1, #2):

[16:13:50] <pchote> antares79: any new thoughts on #13749?
[16:14:34] <antares79> I think it's good. I'm gonna play two or three more games on it, but it looks good

[20:47:11] <antares79> I've been playtesting #13749 all evening and I get a SEGV on at least d2k after about 15min into the game
[20:47:29] <antares79> haven't yet been able to reproduce it on bleed or in other mods, but I'm still trying

Member

pchote commented Aug 17, 2017

My understanding is that @obrakmann has made some progress here, but is currently stalled on an unknown but possibly unrelated crash issue:

From 2017-08-13 (#1, #2):

[16:13:50] <pchote> antares79: any new thoughts on #13749?
[16:14:34] <antares79> I think it's good. I'm gonna play two or three more games on it, but it looks good

[20:47:11] <antares79> I've been playtesting #13749 all evening and I get a SEGV on at least d2k after about 15min into the game
[20:47:29] <antares79> haven't yet been able to reproduce it on bleed or in other mods, but I'm still trying

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Aug 18, 2017

Contributor

Rebased to take advantage of any other Mobile related fixes. Fixed "turn pause".

Contributor

forcecore commented Aug 18, 2017

Rebased to take advantage of any other Mobile related fixes. Fixed "turn pause".

@pchote

I have not tested this extensively (but someone else should!) but the code changes look sensible, and the basics (moving, turning, cancelling while moving or turning) all seem to work as expected.

Can you please squash your fixups in the the appropriate parent commits?

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Aug 19, 2017

Contributor

Squashed

Contributor

forcecore commented Aug 19, 2017

Squashed

@Mailaender

This comment has been minimized.

Show comment
Hide comment
@Mailaender

Mailaender Aug 27, 2017

Member

I played a small test game. No crashes. It seemed at one point a stop command was ignored and the unit was accepting orders only until it moved to its destination. I don't think I can reproduce and I am not sure if that is related. It happened only once with a large group and narrow corridors so units took a detour. Looks good I guess.

Member

Mailaender commented Aug 27, 2017

I played a small test game. No crashes. It seemed at one point a stop command was ignored and the unit was accepting orders only until it moved to its destination. I don't think I can reproduce and I am not sure if that is related. It happened only once with a large group and narrow corridors so units took a detour. Looks good I guess.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Aug 27, 2017

Contributor

I see your bug: force fire on the ground to a place far away so that the unit will move. Then the stop order will be ignored.

Contributor

forcecore commented Aug 27, 2017

I see your bug: force fire on the ground to a place far away so that the unit will move. Then the stop order will be ignored.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Aug 27, 2017

Contributor

In MoveAdjacentTo (or any other activities that may use Move as its inner/Child activity), when Cancel() is called on Move, it will not think it is canceled as Move.cancel returns false. Is it possible to mark Move as "due to be canceled"?

Contributor

forcecore commented Aug 27, 2017

In MoveAdjacentTo (or any other activities that may use Move as its inner/Child activity), when Cancel() is called on Move, it will not think it is canceled as Move.cancel returns false. Is it possible to mark Move as "due to be canceled"?

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Sep 10, 2017

Contributor

[20:47:11] < antares79> I've been playtesting #13749 all evening and I get a SEGV on at least d2k after about 15min into the game
[20:47:29] < antares79> haven't yet been able to reproduce it on bleed or in other mods, but I'm still trying

Sorry for the delay. The crash I encountered is also happening on bleed, just took a bit longer to appear there (~1h).

Contributor

obrakmann commented Sep 10, 2017

[20:47:11] < antares79> I've been playtesting #13749 all evening and I get a SEGV on at least d2k after about 15min into the game
[20:47:29] < antares79> haven't yet been able to reproduce it on bleed or in other mods, but I'm still trying

Sorry for the delay. The crash I encountered is also happening on bleed, just took a bit longer to appear there (~1h).

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Sep 10, 2017

Contributor

Is it possible to mark Move as "due to be canceled"?

Not currently, but I've come to realize that we need something like this. Really it's just required for Move, but since any activity can have Move as a child, it's probably best to bake support for that into the Activity class.

Contributor

obrakmann commented Sep 10, 2017

Is it possible to mark Move as "due to be canceled"?

Not currently, but I've come to realize that we need something like this. Really it's just required for Move, but since any activity can have Move as a child, it's probably best to bake support for that into the Activity class.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 10, 2017

Member

The original goal was for the return value to be that check, and in cases like this Move (or docking, or whatever else) could return true to indicate that they are cancelling, even if they won't be canceled for another few ticks. This detail may not longer hold true in all cases, and would need some testing.

Member

pchote commented Sep 10, 2017

The original goal was for the return value to be that check, and in cases like this Move (or docking, or whatever else) could return true to indicate that they are cancelling, even if they won't be canceled for another few ticks. This detail may not longer hold true in all cases, and would need some testing.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 14, 2017

Member

Any progress here @forcecore?

Member

pchote commented Oct 14, 2017

Any progress here @forcecore?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 14, 2017

Member

The best solution for now may simply be to lie: unconditionally return true; in Move.Cancel, with a comment explaining why. I suspect that the rest of the code will then just work in the way that we want.

Member

pchote commented Oct 14, 2017

The best solution for now may simply be to lie: unconditionally return true; in Move.Cancel, with a comment explaining why. I suspect that the rest of the code will then just work in the way that we want.

@pchote pchote dismissed their stale review via bf9946b Oct 21, 2017

@pchote

pchote approved these changes Oct 21, 2017

I have pushed over your branch to rebase and fix the activity cancellation as mentioned above.

I did some (admittedly limited) testing, and it now seems to behave as desired so 👍

@reaperrr reaperrr merged commit d49c98c into OpenRA:bleed Nov 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@forcecore forcecore deleted the forcecore:MoveParChild branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment