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 the carryall freeze and untangle the afterLandActivity mess #14838

Merged
merged 3 commits into from Nov 2, 2018

Conversation

Projects
None yet
5 participants
@abcdefg30
Copy link
Member

abcdefg30 commented Feb 22, 2018

Fixes #14572.
Supersedes #14595.

This PR removes MovingToResources, MovingToRefinery and MovementCancelled from the INotifyHarvesterAction interface, as those methods are redundant to what ICallForTransport offers.
We also avoid getting stuck in a loop by not creating a new activity and queuing another new one as landAfterActivity right afterwards.

@abcdefg30 abcdefg30 changed the title Fix the carryall freeze and untangle afterLandActivity mess Fix the carryall freeze and untangle the afterLandActivity mess Feb 22, 2018

@abcdefg30 abcdefg30 force-pushed the abcdefg30:carryFreeze branch from 6c9d616 to 387addb Feb 22, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 22, 2018

Should be fixed now.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Feb 22, 2018

This literally breaks the RA2 Chrono harvester.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 22, 2018

Hm, one could work around that by using the ICallForTransport interface, that's not very nice though. On a quick thought: What about having Harvester + its activities only care about INotifyHarvesterAction (without removing the extra interface methods) and having AutoCarryable only care about ICallForTransport (fulfilling the second request behind this PR) with a third trait bridging harvester actions and transport requests?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 4, 2018

with a third trait bridging harvester actions and transport requests?

What about a CallsForTransport bool flag on Harvester instead?

Your proposal might be more sensible, but right now I just can't picture how that bridging trait would look like (let alone how to call it).

@reaperrr reaperrr added this to the Next + 1 milestone Mar 4, 2018

@Unit158

This comment has been minimized.

Copy link
Contributor

Unit158 commented Mar 6, 2018

This should be labeled "Unaddressed comments" not "Unadressed comments"

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 7, 2018

@GraionDilach's point about the chrono harvester invalidates the original motivation for removing INotifyHarvesterAction.MovingToRefinery etc. Considering that, keeping AutoCarryable : INotifyHarvesterAction is IMO the most sensible implementation. Can we revert that change from here?

It may make sense to split INotifyHarvesterAction into two interfaces, one for movement and the other for docking. I assume the later would then be merged into a future docking rework.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Apr 8, 2018

In that case, I would actually also file an issue about axing the interfaces, instantly assigned to the Future milestone with the flashed out locomotor setup being it's prerequisite - technically, the RA2 harvester could jump to any dock (service depot included) and it would make sense to shift the implementation to a locomotor longterm instead of what it is atm.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 29, 2018

Ping @abcdefg30. It would be good to get some progress here both to fix this crash finally for the next release, and also to help clean up the PR queue 😄

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 28, 2018

If we can get this ready and reviewed in time for the playtest, then great. I don't think we can justify delaying the playtest much more specifically for this, however. Bumping to Next + 1.

@pchote pchote modified the milestones: Next release, Next + 1 May 28, 2018

@abcdefg30 abcdefg30 force-pushed the abcdefg30:carryFreeze branch from 387addb to 5614ca4 Aug 31, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Aug 31, 2018

Rebased & updated.

Added a new CarryableHarvester trait that does the bridging between INotifyHarvesterAction and ICallForTransport. (That was previously done in AutoCarryable, so that trait is "untangled" now as well.)

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Aug 31, 2018

This will need an upgrade rule?

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 1, 2018

Yes, that might be a good idea.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:carryFreeze branch from 5614ca4 to 934ff7a Sep 15, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 15, 2018

Rebased and added a rule.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 1, 2018

Needs a rebase, and the update rule should move to the new directory.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:carryFreeze branch from 934ff7a to 3696130 Oct 1, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Oct 1, 2018

Rebased.

var findResources = new FindResources(self);
foreach (var n in notify)
n.MovingToResources(self, moveTo, findResources);
// FindResources cares about calling INotifyHarvesterAction

This comment has been minimized.

@reaperrr

reaperrr Nov 1, 2018

Contributor

'cares about' -> 'takes care of'

self.SetTargetLine(Target.FromCell(self.World, loc.Value), Color.Red);

foreach (var n in notify)
n.MovingToResources(self, loc.Value, findResources);
// FindResources cares about calling INotifyHarvesterAction

This comment has been minimized.

@reaperrr

reaperrr Nov 1, 2018

Contributor

'cares about' -> 'takes care of'

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 1, 2018

Otherwise looks good to me, needs a rebase as well, though.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:carryFreeze branch 2 times, most recently from bd5724d to 9e3c27a Nov 1, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Nov 1, 2018

Updated.

@pchote pchote force-pushed the abcdefg30:carryFreeze branch from 9e3c27a to c807d99 Nov 2, 2018

@pchote pchote force-pushed the abcdefg30:carryFreeze branch from c807d99 to a6b7c1a Nov 2, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 2, 2018

I have pushed an update to this branch:

  • Fix a typo in the second commit's message
  • Rebase
  • Fix the update rule to not refer to a specific line number
@pchote

pchote approved these changes Nov 2, 2018

Copy link
Member

pchote left a comment

Spectated a very long D2K AI game without any noticable issues. Code LGTM.

@pchote pchote merged commit ed7d125 into OpenRA:bleed Nov 2, 2018

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