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

Clean up remaining Capture API issues #15737

Merged
merged 3 commits into from Nov 3, 2018

Conversation

Projects
None yet
4 participants
@pchote
Copy link
Member

pchote commented Oct 26, 2018

Fixes #15076.
Fixes #15697.

I have tested:

  • OrderTargeter changes
  • Capture Lua API change (but note #15736).
  • Oil Derricks (GivesCashOnCapture).
  • Husk repair (TransformOnCapture).

@pchote pchote added this to the Next Release milestone Oct 26, 2018

var c = target.Info.TraitInfoOrDefault<CapturableInfo>();
if (c == null || !c.CanBeTargetedBy(self, target.Owner))
var captureManagerInfo = target.Info.TraitInfoOrDefault<CaptureManagerInfo>();
if (captureManagerInfo == null || !captureManagerInfo.CanBeTargetedBy(target, self, captures))

This comment has been minimized.

@reaperrr

reaperrr Nov 1, 2018

Contributor

I've been wondering about this with ConditionManager already:

Considering that the *Manager traits are neither conditional nor do the logics using them work when they're missing, wouldn't it actually be more modder-friendly to drop the null checks and crash if they're missing?
The way I see it, in cases like this modders will wonder why capturing doesn't work, instead of getting an exception that tells them that CaptureManager is missing.

Edit: Not a blocker for this PR either way, though.

This comment has been minimized.

@pchote

pchote Nov 2, 2018

Member

If this is caught by the linter and optionally mod startup, then I guess this would be fine.

@SoScared

This comment has been minimized.

Copy link
Member

SoScared commented Nov 1, 2018

Tested with a good chunk of games played. Encountered no issues.

@pchote pchote force-pushed the pchote:multiple-captures branch from d23dfcd to ab0e8f2 Nov 2, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 2, 2018

Updated and rebased.

@abcdefg30 abcdefg30 merged commit 3d6b170 into OpenRA:bleed Nov 3, 2018

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.

Copy link
Member

abcdefg30 commented Nov 3, 2018

@pchote pchote deleted the pchote:multiple-captures branch Nov 18, 2018

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