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

Refactor resource harvesting logic. #16430

Merged
merged 2 commits into from Apr 22, 2019

Conversation

@tovl
Copy link
Contributor

commented Apr 21, 2019

Split from #16408.

Fixes #16412
Fixes #13260

Added a temporary workaround for #12817, because otherwise without the pathfinding changes in #16408 this would make the problem worse.

@tovl tovl force-pushed the tovl:refactorharvest branch from fcc8d5c to a1e1930 Apr 21, 2019

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

left a comment

Code changes look good, and I didn't notice any bad behaviour testing in TD.

@pchote pchote added the PR: Needs +2 label Apr 22, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

What's the plan with the temporary fix here? Revert it in #16408? Then I would like too keep #13260 open and add a comment about the temporary fix to the issue.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

The temporary fix to #12817 will be reverted in #16408. This temporary fix does not unfix #13260 because there is now an explicit check for harvestable resources after the move as well.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

What I request is that we don't close #13260 here (remove the "Fixes ..." from this PR'S description) when the plan is to revert this change later. I don't really understand what you mean with "does not unfix". Do you mean that reverting the fix does not "unifx" the issue? The problem I have is that currently this PR and #16408 "fix" #13260, and only the "real" fix should close the issue IMO.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Sorry my bad, I misunderstood what issues this PR closes.

@Punsho
Punsho approved these changes Apr 22, 2019
Copy link
Contributor

left a comment

I ran some tests and harvesters only get temporally stuck when they exceed the 3 harvs per refinery limit. As intended.

@matjaeck
Copy link
Contributor

left a comment

Created a test map and #13260 seems fixed, too.

@reaperrr reaperrr merged commit ffbee7e into OpenRA:bleed Apr 22, 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
5 participants
You can’t perform that action at this time.