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

Merge Rearm and Repair activities into single Resupply activity #16352

Merged
merged 5 commits into from Apr 29, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Mar 24, 2019

While going through my TODO-list for the resupply refactor, I noticed that this part actually doesn't have any real dependencies left.

This finally allows for repairs and rearming to run at the same time, which can be tested on the RA minelayer.

Closes #9613.

@reaperrr reaperrr added this to the Next Release milestone Mar 24, 2019

@obrakmann
Copy link
Contributor

left a comment

RepairableNear is currently broken

OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Rebased and updated.

OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Resupply.cs Outdated Show resolved Hide resolved
@obrakmann
Copy link
Contributor

left a comment

This looks ok to me now. The only remaining issue I see already exists on bleed and would require re-writing Repairable almost completely.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

The only remaining issue I see already exists on bleed and would require re-writing Repairable almost completely.

Since I plan to move most innards (order handling etc.) of Repairable to a general ResupplyManager, it would be good to know that issue beforehand, so I won't have to redo most of it after I complete and file that PR.

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch from 66e9dec to f946c5b Mar 31, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

Updated (and rebased, as that Cargo crash we just fixed can crash the shellmap early if you're not fast enough).
Also gave the RA minelayer a quick test to make sure it still works after the latest changes.

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch 2 times, most recently from bb8b33a to 3c20158 Mar 31, 2019

@obrakmann

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Since I plan to move most innards (order handling etc.) of Repairable to a general ResupplyManager, it would be good to know that issue beforehand, so I won't have to redo most of it after I complete and file that PR.

The issue that if you sent a unit to repair itself and immediately queue another command afterwards, that second command will be executed after the unit reaches the repair pad. After completing that order it will then drive back to the repair pad and execute the AfterReachActivities. The cause is that the latter get queued from a CallFunc. This was done to enable carryalls carrying units to repair pads.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

See #13390 (comment), bulllet point no. 3 and gif.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

It looks like Repairable.ResolveOrder is manually wrangling/mangling activities - moving all the the move-to/carryall stuff inside the Resupply activity (or a new activity) will bring this in line with our best practice, and queuing them as children instead of top-level activities will fix the ordering issues.

This may be best left to a separate PR, since this is all aside from the main goal here.

@matjaeck
Copy link
Contributor

left a comment

This finally allows for repairs and rearming to run at the same time, which can be tested on the RA minelayer.

Works as advertised.

Aborting the resupply activity by moving away / stop order will not cancel the service depot animation in RA. Same in TS but only when moving away (cant stop repairing).

@reaperrr reaperrr referenced this pull request Apr 12, 2019
1 of 5 tasks complete

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch from 3c20158 to 09fd1b7 Apr 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Aborting the resupply activity by moving away / stop order will not cancel the service depot animation in RA. Same in TS but only when moving away (cant stop repairing).

Should be fixed now.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

Dismissing the review in the light of pchote's comment.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

@pchote It doesn't sound like we're talking about the same issue here.
What I meant was simply that since actors can only execute one activity at a time, it's normal that an (unqueued) attack order would cancel the Resupply activity, since in that case AttackBase.AttackTarget does exactly that, cancel the current activity before queueing the attack activity. And that's the only issue I see in @matjaeck's GIFs.

To be honest, I also have no idea what inconsistent state you're referring to. Your code changes don't seem to make a visible difference, though admittedly I don't even know what issue I'm supposed to look out for.

Apart from that, one could argue that actors which aborted their resupply due to attacking something should restart it if they become idle while still on the resupplier, but that would be a task for the ResupplyManager so I'd like to leave that for #16446.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

By inconsistent state I mean that cancelling the activity at the wrong time can leave the actor in a state (in this case its location) that would otherwise be considered as invalid (in this case sitting within the footprint of a building). If you cancel a move order while an actor is between two cells it will keep moving until it reaches the next (sub)cell on its path, and if you cancel the TD harvester unloading it will undock itself before doing whatever you asked next. The same should happen here.

The code snippet I posted above makes the actor undock itself and move to a free cell next to the pad when cancelled early. The pad is then free for other actors to use, and there is no need to worry about trying to recover/restart the resupply.

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch from 5c9e6fd to 517011a Apr 28, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

OK, updated.
This doesn't work all the time - it fails if the Stop or Attack order is given a bit before entering the 'closeEnough' range - but this is already an improvement over bleed and fixing this more robustly would probably require refactoring the activity queueing in Repairable, and that's out of scope for this PR.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

I assume that your planned docking code will make the proper fix much cleaner, so IMO its fine to add a // HACK: comment and defer the proper fix until then.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Something is wrong with the repair animation is ts, but looks good otherwise. Turreted actors that are aiming before docking can fire on their target while repairing btw.

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch from 517011a to 682e85e Apr 28, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Updated.

@matjaeck If it's only in TS, it's probably either a problem with the repair overlay or simply a yaml setup issue. I'll take a look later, but I think that doesn't need to be fixed here.

reaperrr added 4 commits Mar 24, 2019
Merge INotifyRearm/Repair into INotifyResupply
And streamline its notify methods.
Also cache INotifyResupply traits at beginning
of Resupply activity.
Remove ResupplyAircraft and AllowYieldingReservation
The few extra things those two activities did can be done
in Resupply, making them redundant.

@reaperrr reaperrr force-pushed the reaperrr:resupply-activity branch from 682e85e to 73044ef Apr 28, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Updated, was a small oversight in the overlay. Fixed.

@matjaeck
Copy link
Contributor

left a comment

Thanks for fixing it here. LGTM from the ingame perspective.

@pchote
pchote approved these changes Apr 29, 2019
Copy link
Member

left a comment

This is a good step in the right direction for fixing the long-bogus repair/rearm code. It doesn't address all the problems, but its not reasonable to expect it to. I have filed #16462, #16463 and #16464 to be addressed in followup PRs. Any other remaining issues or regressions can also be left for followups.

@pchote pchote merged commit 543d46f into OpenRA:bleed Apr 29, 2019

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
6 participants
You can’t perform that action at this time.