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

#14542 fix carry all path #14558

Merged
merged 2 commits into from Dec 25, 2017

Conversation

Projects
None yet
4 participants
@jongleur1983
Contributor

jongleur1983 commented Dec 21, 2017

This PR is an easy fix to issue #14542: When the harvester was explicitly ordered to the refinery before being taken by the CarryAll, the CarryAll loaded the harvester (as intended), but did a detour to (0,0), unloaded, reloaded and then moved to the refinery it should have moved directly.
This was due to the harvester issuing the wrong coordinate from order.TargetLocation.

As @penev92 and @pchote engage in a longer discussion about what should or should not happen here - in short, mid or long term goals - I'll leave this PR here, happy to take suggestions for what to do different or what to do next.

@penev92 penev92 added this to the Next release milestone Dec 22, 2017

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Dec 22, 2017

Member

Please squash the first 2 commits and fix the third's message.

Member

penev92 commented Dec 22, 2017

Please squash the first 2 commits and fix the third's message.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Harvester.cs Outdated

jongleur1983 added a commit to jongleur1983/OpenRA that referenced this pull request Dec 23, 2017

OpenRA#14558: MovingToRefinery takes actor instead of CPos
DeliveryOffset (previously added by the harvester) is now taken into account by the AutoCarryable

jongleur1983 added a commit to jongleur1983/OpenRA that referenced this pull request Dec 23, 2017

OpenRA#14558: MovingToRefinery takes actor instead of CPos
DeliveryOffset (previously added by the harvester) is now taken into account by the AutoCarryable
fix whitespaces
@pchote

LGTM now, thanks 👍

Can you please squash the last two commits together?

@pchote pchote added the PR: Needs +2 label Dec 23, 2017

@abcdefg30

Looks like this uncovers another freeze (possibly the stack overflow you mentioned earlier?), not really related to the fix though.

@jongleur1983 jongleur1983 dismissed stale reviews from abcdefg30 and pchote via c26ec1d Dec 24, 2017

jongleur1983 added a commit to jongleur1983/OpenRA that referenced this pull request Dec 24, 2017

add me (jongleur1983) to AUTHORS
OpenRA#14558: MovingToRefinery takes actor instead of CPos

DeliveryOffset (previously added by the harvester) is now taken into account by the AutoCarryable
fix whitespaces

jongleur1983 added some commits Dec 22, 2017

#14542: order CarrryAll to the target's location, not to 0,0
(which is in order.TargetLocation
#14542: don't use deprecated TargetActor property, replace by

Target.Actor.Location
add me (jongleur1983) to AUTHORS
#14558: MovingToRefinery takes actor instead of CPos

DeliveryOffset (previously added by the harvester) is now taken into account by the AutoCarryable
fix whitespaces
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Dec 24, 2017

Member

Looks like this uncovers another freeze (possibly the stack overflow you mentioned earlier?), not really related to the fix though.

Can you or @jongleur1983 please file a new issue with steps to reproduce?

@jongleur1983 note that GitHub doesn't notify us when you push changes to a PR, so make sure to leave a comment when you push fixes so that we can notice them!

Member

pchote commented Dec 24, 2017

Looks like this uncovers another freeze (possibly the stack overflow you mentioned earlier?), not really related to the fix though.

Can you or @jongleur1983 please file a new issue with steps to reproduce?

@jongleur1983 note that GitHub doesn't notify us when you push changes to a PR, so make sure to leave a comment when you push fixes so that we can notice them!

@jongleur1983

This comment has been minimized.

Show comment
Hide comment
@jongleur1983

jongleur1983 Dec 24, 2017

Contributor

If nobody does before I hope to find the time tonight. Couldn't reproduce the StackOverflow issue any more after my fix, but as @abcdefg30 found it, too, it's there obviously.
But if I'm right this issue itself shouldn't be blocked any more.

Contributor

jongleur1983 commented Dec 24, 2017

If nobody does before I hope to find the time tonight. Couldn't reproduce the StackOverflow issue any more after my fix, but as @abcdefg30 found it, too, it's there obviously.
But if I'm right this issue itself shouldn't be blocked any more.

@abcdefg30

Fixes carryallys going to (0, 0). Filed #14572 for the other crash.

@abcdefg30 abcdefg30 merged commit 4be5931 into OpenRA:bleed Dec 25, 2017

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.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Dec 25, 2017

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