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

Conditional (External)Capturable #13516

Merged
merged 1 commit into from Nov 15, 2017

Conversation

Projects
None yet
5 participants
@forcecore
Contributor

forcecore commented Jun 16, 2017

Capturable and External capturable are now conditional. Potentially, Building.Lock() can be replaced by this modification, not applied in this PR at the moment.

Needed for "mind control" system in RA2. Suppose there is a building-mind-controller of player A. It will capture the structure of player B. Then the third player C decides to capture it with an engineer. The building will be C's, for now. But if the mind controller dies, at the time of mind control, it was A's so the building will be returned to A's. (Can't hard code this situation to transfer ownership on mind control because in Kane's Wrath, a mind controller can snatch what someone has already controlled so capturing isn't the only way of ownership change for units that are already mind controlled)

To prevent that, OnOwnerChange notifier must be modified to deliver extra information on what activity changed the ownership so that eventually A's mind controller knows who the building should be returned to.

Another way is to forbid the capture of mind controlled buildings by making Capturable traits conditional.

@forcecore forcecore changed the title from (External)Capturable are now conditional to Conditional (External)Capturable Jun 16, 2017

@Smittytron

This comment has been minimized.

Show comment
Hide comment
@Smittytron

Smittytron Jun 16, 2017

Contributor

I’ve been trying to get engineers to ‘salvage’ a husk by using external capture and cash trickler. The problem is the husk will transform into a restored unit as if a mechanic captured it, as opposed to being consumed as intended.

Will conditions be able to help with this?

Contributor

Smittytron commented Jun 16, 2017

I’ve been trying to get engineers to ‘salvage’ a husk by using external capture and cash trickler. The problem is the husk will transform into a restored unit as if a mechanic captured it, as opposed to being consumed as intended.

Will conditions be able to help with this?

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jun 16, 2017

Contributor

That would need something else. TransformOnCapture trait needs to be conditional too and capture should GRANT a condition too. Or, support multiple capturable traits with filters (like the filter in DliversCash trait)? With filter support for TransformOnCapture, too

Contributor

forcecore commented Jun 16, 2017

That would need something else. TransformOnCapture trait needs to be conditional too and capture should GRANT a condition too. Or, support multiple capturable traits with filters (like the filter in DliversCash trait)? With filter support for TransformOnCapture, too

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jun 17, 2017

Contributor

@Smittytron Actually, it turns out I need TransformOnCapture filtered for my mod too! I'll make a PR when done.

Contributor

forcecore commented Jun 17, 2017

@Smittytron Actually, it turns out I need TransformOnCapture filtered for my mod too! I'll make a PR when done.

@pchote

A few not too difficult requests:

Show outdated Hide outdated OpenRA.Mods.Common/AI/HackyAI.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/AI/HackyAI.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Capturable.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/ExternalCapturable.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/ExternalCapturable.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Capturable.cs Outdated
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 21, 2017

Member

Re mind control: Refer to the TemporaryOwnerManager trait, which already solves some of those problems for D2K's Deviator ("mind control") logic.

Member

pchote commented Jul 21, 2017

Re mind control: Refer to the TemporaryOwnerManager trait, which already solves some of those problems for D2K's Deviator ("mind control") logic.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Aug 15, 2017

Contributor

Code updated.
TemporaryOwnerManager does what it is supposed to by overriding the mind control by capture. On the other hand, "mind control vs capture, which one wins?" depends on how you interpret engineers' capture action

  • If you interpret that engineers are killing all the guys inside with some sort of a martial art that is useless outside buildings, then TemporaryOwnerManager is the solution
  • Then in my interpretation, where I interpret the capture as bribing the guys inside (when you sell the captured structure, you don't get the engineer back but there are several guys that have turned to your side!), mind control puppets shouldn't be bribed xD. In this interpretation, capture should be disabled :)
Contributor

forcecore commented Aug 15, 2017

Code updated.
TemporaryOwnerManager does what it is supposed to by overriding the mind control by capture. On the other hand, "mind control vs capture, which one wins?" depends on how you interpret engineers' capture action

  • If you interpret that engineers are killing all the guys inside with some sort of a martial art that is useless outside buildings, then TemporaryOwnerManager is the solution
  • Then in my interpretation, where I interpret the capture as bribing the guys inside (when you sell the captured structure, you don't get the engineer back but there are several guys that have turned to your side!), mind control puppets shouldn't be bribed xD. In this interpretation, capture should be disabled :)

@pchote pchote dismissed their stale review Aug 19, 2017

Code updated.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 14, 2017

Member

Needs a rebase.

Member

abcdefg30 commented Sep 14, 2017

Needs a rebase.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Sep 15, 2017

Contributor

Rebased.

Contributor

forcecore commented Sep 15, 2017

Rebased.

Show outdated Hide outdated OpenRA.Mods.Common/Traits/Captures.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/Capturable.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/ExternalCapturable.cs Outdated
@@ -128,7 +128,7 @@ public override bool CanTargetActor(Actor self, Actor target, TargetModifiers mo
{
var c = target.TraitOrDefault<ExternalCapturable>();
var canTargetActor = c != null && !c.CaptureInProgress && c.Info.CanBeTargetedBy(self, target.Owner);
var canTargetActor = c != null && !c.CaptureInProgress && c.CanBeTargetedBy(self, target.Owner);

This comment has been minimized.

@pchote

pchote Sep 16, 2017

Member

CanTargetFrozenActor is still checking the ExternalCapturableInfo, and will need to be changed to match CaptureOrderTargeter.

@pchote

pchote Sep 16, 2017

Member

CanTargetFrozenActor is still checking the ExternalCapturableInfo, and will need to be changed to match CaptureOrderTargeter.

This comment has been minimized.

@forcecore

forcecore Sep 21, 2017

Contributor

Now I think that it is correct not to change CanTargetFronzenActor, as the fog obscures the capturableness. Captures.cs is reverted.

@forcecore

forcecore Sep 21, 2017

Contributor

Now I think that it is correct not to change CanTargetFronzenActor, as the fog obscures the capturableness. Captures.cs is reverted.

This comment has been minimized.

@pchote

pchote Sep 21, 2017

Member

It needs to account for the state at the time the actor was frozen, otherwise you leak info through the fog. I have a set of prs lined up to solve this, but so far #13995 has not had any review attention.

@pchote

pchote Sep 21, 2017

Member

It needs to account for the state at the time the actor was frozen, otherwise you leak info through the fog. I have a set of prs lined up to solve this, but so far #13995 has not had any review attention.

This comment has been minimized.

@pchote

pchote Sep 24, 2017

Member

On further thought, I don't think we should hold up this PR over the future frozen actor fixes.

So long as this PR doesn't change the behaviour for non-disabled traits then this can still be useful for regular units that don't get frozen under fog.

@pchote

pchote Sep 24, 2017

Member

On further thought, I don't think we should hold up this PR over the future frozen actor fixes.

So long as this PR doesn't change the behaviour for non-disabled traits then this can still be useful for regular units that don't get frozen under fog.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 24, 2017

Member

Resetting tags - this has now changed enough to want a complete re-review.

Member

pchote commented Sep 24, 2017

Resetting tags - this has now changed enough to want a complete re-review.

@pchote

I have pushed over your branch to rebase and fix the comments as mentioned above.

👍 now that these fixes are in place.

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

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Oct 22, 2017

Member

All good and all, but ExternalCaptures has issues with frozen actors that have their ExternalCapturable trait disabled, as in you can issue and order and actually capture the actor. Not possible to issue an order on non-frozen actors though, and Capturable works for both (almost) as expected (there is some derpiness with frozen actors, but nothing game-breaking).

Simple test case: add a random ungranted condition to RA structures' ExternalCapturable, then try to capture anything that is frozen.

Member

penev92 commented Oct 22, 2017

All good and all, but ExternalCaptures has issues with frozen actors that have their ExternalCapturable trait disabled, as in you can issue and order and actually capture the actor. Not possible to issue an order on non-frozen actors though, and Capturable works for both (almost) as expected (there is some derpiness with frozen actors, but nothing game-breaking).

Simple test case: add a random ungranted condition to RA structures' ExternalCapturable, then try to capture anything that is frozen.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 22, 2017

Member

@penev92: See the comment int ExternalCaptures.cs.

Fixing the frozen actor state requires a massive rework, which I have already started with #13995 and #14189. At the current rate of ~1 month merge delay per PR then we can expect those changes to be finished by around this time next year 😢

In this PR we only really have two options:

  • Merge it so that it can be used for non-frozen actors.
  • Close the PR and maybe revisit it in the future after the frozen actor logic has been finished.
Member

pchote commented Oct 22, 2017

@penev92: See the comment int ExternalCaptures.cs.

Fixing the frozen actor state requires a massive rework, which I have already started with #13995 and #14189. At the current rate of ~1 month merge delay per PR then we can expect those changes to be finished by around this time next year 😢

In this PR we only really have two options:

  • Merge it so that it can be used for non-frozen actors.
  • Close the PR and maybe revisit it in the future after the frozen actor logic has been finished.
@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 15, 2017

Member

👍 with the note that it shouldn't be used on buildings with ExternalCapturable.

Member

penev92 commented Nov 15, 2017

👍 with the note that it shouldn't be used on buildings with ExternalCapturable.

@penev92 penev92 merged commit c762453 into OpenRA:bleed Nov 15, 2017

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
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 Nov 15, 2017

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