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

fixs for Refinery spawned harvesters should prioritize lower ore #14874

Merged
merged 1 commit into from Mar 10, 2018

Conversation

Projects
None yet
8 participants
@CJure
Copy link
Contributor

CJure commented Mar 3, 2018

Closes #14827 Refinery spawned harvesters should prioritize lower

@CJure CJure changed the title fix for Refinery spawned harvesters should prioritize lower ore #14827 Closes #14827 Refinery spawned harvesters should prioritize lower ore Mar 3, 2018

.gitignore Outdated
@@ -11,7 +11,7 @@ obj
*.CodeAnalysisLog.xml
*.lastcodeanalysissucceeded
_ReSharper.*/
/.vs
/.vs

This comment has been minimized.

@GraionDilach

GraionDilach Mar 3, 2018

Contributor

Revert this one.

.gitignore Outdated

# IntelliJ files
.idea

This comment has been minimized.

@GraionDilach

GraionDilach Mar 3, 2018

Contributor

And these as well.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 3, 2018

OpenRA.Mods.Common/Activities/FindResources.cs:L169: [SA1513] Statements or elements wrapped in curly brackets must be followed by a blank line.
OpenRA.Mods.Common/Activities/FindResources.cs:L173: [SA1508] A closing curly bracket must not be preceded by a blank line.

@fruestueck

This comment has been minimized.

Copy link
Contributor

fruestueck commented Mar 3, 2018

Welcome CJure,
you can use make.cmd check before uploading a commit to find and avoid style violations.
I'd also avoid #[issueNr] in a PR-title and in commit-descriptionsname.

@CJure CJure changed the title Closes #14827 Refinery spawned harvesters should prioritize lower ore fixs for Refinery spawned harvesters should prioritize lower ore Mar 3, 2018

@CJure

This comment has been minimized.

Copy link
Contributor Author

CJure commented Mar 3, 2018

@GraionDilach how can I commit without .gitignore if I don't add it then some other visual studio file will be added
@fruestueck tnx for make.cmd, didn't know about that. Removd PR from title.

@CJure CJure force-pushed the CJure:bleed branch from c0698c2 to 4600d4e Mar 3, 2018

@CJure CJure closed this Mar 3, 2018

@CJure CJure force-pushed the CJure:bleed branch from 4600d4e to 42f1db0 Mar 3, 2018

@CJure CJure reopened this Mar 3, 2018

@Smittytron
Copy link
Contributor

Smittytron left a comment

Tested in game. Works as advertised, harvesters no longer run all around the refinery upon spawning.

return harv.LastLinkedProc.Location + harv.LastLinkedProc.Trait<IAcceptResources>().DeliveryOffset;
else if (harv.LinkedProc != null)
return harv.LinkedProc.Location + harv.LinkedProc.Trait<IAcceptResources>().DeliveryOffset;
else if (harv.LastOrderLocation != null)

This comment has been minimized.

@pchote

pchote Mar 4, 2018

Member

Why was this moved to lowest priority? Won't this break all the cases where we don't want the closest ore?

This comment has been minimized.

@reaperrr

reaperrr Mar 4, 2018

Contributor

I agree. Using DeliveryOffset instead of Location should be the only change here.

LastOrderLocation needs to remain on top, else we'll definitely run into regressions.

@CJure CJure force-pushed the CJure:bleed branch from b7b3508 to 279b091 Mar 5, 2018

@CJure

This comment has been minimized.

Copy link
Contributor Author

CJure commented Mar 5, 2018

Changed back

@@ -152,5 +152,16 @@ public override IEnumerable<Target> GetTargets(Actor self)
{
yield return Target.FromCell(self.World, self.Location);
}

public CPos GetFromSearchLocation(Actor self)

This comment has been minimized.

@Unit158

Unit158 Mar 6, 2018

Contributor

This should be private.


public CPos GetFromSearchLocation(Actor self)
{
if (harv.LastOrderLocation != null)

This comment has been minimized.

@Unit158

Unit158 Mar 6, 2018

Contributor

Given that this is a nullable type, I'd check if it has a value

public CPos GetFromSearchLocation(Actor self)
{
if (harv.LastOrderLocation != null)
return (CPos)harv.LastOrderLocation;

This comment has been minimized.

@Unit158

Unit158 Mar 6, 2018

Contributor

and get the value here instead of casting

@CJure CJure force-pushed the CJure:bleed branch from 279b091 to 0440094 Mar 7, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

After the comment is addressed.

@@ -152,5 +152,16 @@ public override IEnumerable<Target> GetTargets(Actor self)
{
yield return Target.FromCell(self.World, self.Location);
}

private CPos GetFromSearchLocation(Actor self)

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 9, 2018

Member

No explicit private keyword, please (that is the default already). Can we also rename this to GetSearchFromLocation? It makes more sense imho and the local variable is also called searchFromLoc.

@CJure CJure dismissed stale reviews from abcdefg30 and reaperrr via dbb1a69 Mar 10, 2018

@CJure CJure force-pushed the CJure:bleed branch from dbb1a69 to a14a55d Mar 10, 2018

@reaperrr reaperrr merged commit 311cd52 into OpenRA:bleed Mar 10, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Mar 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.