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

Replace Order TargetActor/TargetCell for serialized orders #14189

Merged
merged 3 commits into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Oct 15, 2017

Followup to #13995: this carries the new Target field through the networking code to where it can be used by ResolveOrder.

As traits are updated to use order.Target they will get free error checking (TargetType.Actor is guaranteed to be a valid living actor, TargetType.Location is guaranteed to not be the placeholder CPos.Zero, etc) and proper support for frozen actors. This will make it possible to split #13363 into smaller more testable PRs starting after this is merged.

@pchote pchote requested review from chrisforbes, reaperrr and rob-v Oct 15, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 15, 2017

Member

Chance of regressions: high. Things that could regress: anything that resolves orders. Regressions should be NRE crashes, so they will at least be obvious.

Member

pchote commented Oct 15, 2017

Chance of regressions: high. Things that could regress: anything that resolves orders. Regressions should be NRE crashes, so they will at least be obvious.

@pchote pchote added this to the Next release milestone Oct 15, 2017

@rob-v

lgtm, one question/issue.

Show outdated Hide outdated OpenRA.Game/Network/Order.cs Outdated
Show outdated Hide outdated OpenRA.Game/Network/Order.cs Outdated
@penev92

The second commit highlights some pretty obvious inconsistencies in our IsValidOrder methods, but I can close my eyes here as while it is a minor fix it is out of the scope of the PR.

Show outdated Hide outdated OpenRA.Game/Network/Order.cs Outdated
var targetLocation = (CPos)(flags.HasField(OrderFields.TargetLocation) ? r.ReadInt2() : int2.Zero);
Actor subject = null;
if (world != null)
TryGetActorFromUInt(world, subjectId, out subject);

This comment has been minimized.

@penev92

penev92 Oct 17, 2017

Member

Not really related, but GH won't let me comment where I want to:
the uint.MaxValue in this method seems completely redundant. Unless I'm mistaken, can you take care of it in this PR?

@penev92

penev92 Oct 17, 2017

Member

Not really related, but GH won't let me comment where I want to:
the uint.MaxValue in this method seems completely redundant. Unless I'm mistaken, can you take care of it in this PR?

This comment has been minimized.

@pchote

pchote Oct 18, 2017

Member

I'd prefer not to do that here - this has a bad enough regression potential as it is.

@pchote

pchote Oct 18, 2017

Member

I'd prefer not to do that here - this has a bad enough regression potential as it is.

Show outdated Hide outdated OpenRA.Game/Network/Order.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

Updated.

Member

pchote commented Oct 18, 2017

Updated.

@rob-v

rob-v approved these changes Oct 18, 2017

lgtm 👍

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

@pchote pchote added the PR: Needs +2 label Oct 20, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 20, 2017

Member

Updated.

Member

pchote commented Oct 20, 2017

Updated.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Oct 26, 2017

Member

👍

Member

penev92 commented Oct 26, 2017

👍

@penev92 penev92 merged commit b8326bf into OpenRA:bleed Oct 26, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@penev92
Member

penev92 commented Oct 26, 2017

@reaperrr reaperrr added this to the Release 20180218 milestone Mar 8, 2018

@pchote pchote deleted the pchote:target-orders-2 branch Apr 28, 2018

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