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

TransformTypes filter for TransformOnCapture #13523

Merged
merged 1 commit into from Nov 12, 2017

Conversation

Projects
None yet
5 participants
@forcecore
Contributor

forcecore commented Jun 17, 2017

Not all captures induce transform. Hence, filter introduced.

  • In #13516 I saw that I'm not the only one who needs the filter. @Smittytron
  • https://www.youtube.com/watch?v=Dh6vEx6BqNg : Use on my mod. Engineers do a normal capture while infesters do a transforming capture.
  • The default value is defined as husk, as the only use so far in official mods is husk revival.

There might be some more interesting usages, such as building a very quick initial scaffolding then letting the engineer finish the construction.

This was rebased and fixed up by @penev92 on 11.11.2017

@forcecore forcecore changed the title from Introduced TransformTypes for TransformOnCapture to TransformTypes filter for TransformOnCapture Jun 17, 2017

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jun 17, 2017

Contributor

Removed TransformTypes and the check is done on CaptureTypes instead. It wouldn't hurt modders to make them put extra types to trigger TransformOnCapture.

Contributor

forcecore commented Jun 17, 2017

Removed TransformTypes and the check is done on CaptureTypes instead. It wouldn't hurt modders to make them put extra types to trigger TransformOnCapture.

@pchote

A couple of minor style nits:

Show outdated Hide outdated OpenRA.Mods.Common/Traits/TransformOnCapture.cs Outdated
Show outdated Hide outdated OpenRA.Mods.Common/Traits/TransformOnCapture.cs Outdated
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 22, 2017

Member

It looks like we will need to expand Captureable to support multiple multiple capture types and/or change Captures to work with multiple Capturable traits on a target before this will be usable ingame.

This otherwise looks good, in theory, aside from the nits above.

Member

pchote commented Jul 22, 2017

It looks like we will need to expand Captureable to support multiple multiple capture types and/or change Captures to work with multiple Capturable traits on a target before this will be usable ingame.

This otherwise looks good, in theory, aside from the nits above.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Sep 15, 2017

Contributor

I found some bugs with this on my mod, probably due to confusing usage. I need to do some more works to get this straight.

Contributor

forcecore commented Sep 15, 2017

I found some bugs with this on my mod, probably due to confusing usage. I need to do some more works to get this straight.

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Oct 2, 2017

Contributor

Rebased, review reflected.

I think my confusion of YAML part really came from trying to filter the Event type (much like DeployType in #13868) with the CapturableType (husk/building).

I tried separating (External)Capturable trait but now they need priority when capturable by multiple captures filter. Any of them will do as long as they get captured so I don't think that the separation/priority is necessary unless we are to have a separate targeting cursor for those actions. I think more thought should be put before actually allowing multiple capturable types.

Contributor

forcecore commented Oct 2, 2017

Rebased, review reflected.

I think my confusion of YAML part really came from trying to filter the Event type (much like DeployType in #13868) with the CapturableType (husk/building).

I tried separating (External)Capturable trait but now they need priority when capturable by multiple captures filter. Any of them will do as long as they get captured so I don't think that the separation/priority is necessary unless we are to have a separate targeting cursor for those actions. I think more thought should be put before actually allowing multiple capturable types.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 21, 2017

Member

The problem I raised before still stands: if an actor only supports a single capture type then I don't understand how you can trigger different effects on different types.

I want to see this feature completed, so I created #14230 to add the missing support. After rebasing, the following testcase then works here:

diff --git a/mods/cnc/rules/husks.yaml b/mods/cnc/rules/husks.yaml
index 74532fea21..b585883760 100644
--- a/mods/cnc/rules/husks.yaml
+++ b/mods/cnc/rules/husks.yaml
@@ -47,8 +47,15 @@ BGGY.Husk:
        Inherits: ^Husk
        Tooltip:
                Name: Nod Buggy (Destroyed)
+       Capturable:
+               Types: husk, husk2 # not possible on current bleed
        TransformOnCapture:
+               CaptureTypes: husk
                IntoActor: bggy
+       TransformOnCapture@2:
+               CaptureTypes: husk2
+               IntoActor: bike
+               ForceHealthPercentage: 25
        RenderSprites:
                Image: bggy.destroyed
 
diff --git a/mods/cnc/rules/infantry.yaml b/mods/cnc/rules/infantry.yaml
index 872965544d..697184f2c4 100644
--- a/mods/cnc/rules/infantry.yaml
+++ b/mods/cnc/rules/infantry.yaml
@@ -23,6 +23,9 @@ E1:
        WithInfantryBody:
                IdleSequences: idle1,idle2,idle3,idle4
                DefaultAttackSequence: shoot
+       Captures:
+               CaptureTypes: husk2
+               PlayerExperience: 50
 
 E2:
        Inherits: ^Soldier
Member

pchote commented Oct 21, 2017

The problem I raised before still stands: if an actor only supports a single capture type then I don't understand how you can trigger different effects on different types.

I want to see this feature completed, so I created #14230 to add the missing support. After rebasing, the following testcase then works here:

diff --git a/mods/cnc/rules/husks.yaml b/mods/cnc/rules/husks.yaml
index 74532fea21..b585883760 100644
--- a/mods/cnc/rules/husks.yaml
+++ b/mods/cnc/rules/husks.yaml
@@ -47,8 +47,15 @@ BGGY.Husk:
        Inherits: ^Husk
        Tooltip:
                Name: Nod Buggy (Destroyed)
+       Capturable:
+               Types: husk, husk2 # not possible on current bleed
        TransformOnCapture:
+               CaptureTypes: husk
                IntoActor: bggy
+       TransformOnCapture@2:
+               CaptureTypes: husk2
+               IntoActor: bike
+               ForceHealthPercentage: 25
        RenderSprites:
                Image: bggy.destroyed
 
diff --git a/mods/cnc/rules/infantry.yaml b/mods/cnc/rules/infantry.yaml
index 872965544d..697184f2c4 100644
--- a/mods/cnc/rules/infantry.yaml
+++ b/mods/cnc/rules/infantry.yaml
@@ -23,6 +23,9 @@ E1:
        WithInfantryBody:
                IdleSequences: idle1,idle2,idle3,idle4
                DefaultAttackSequence: shoot
+       Captures:
+               CaptureTypes: husk2
+               PlayerExperience: 50
 
 E2:
        Inherits: ^Soldier
@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 11, 2017

Member

#14230 was merged, so no more dependencies.

Member

penev92 commented Nov 11, 2017

#14230 was merged, so no more dependencies.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 11, 2017

Member

Rebased, renamed TransformOnCaptureInfo.Type to TransformOnCaptureInfo.CaptureType and squashed all 4 commits into 1.

Test case provided by @pchote works.
👍

Member

penev92 commented Nov 11, 2017

Rebased, renamed TransformOnCaptureInfo.Type to TransformOnCaptureInfo.CaptureType and squashed all 4 commits into 1.

Test case provided by @pchote works.
👍

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 12, 2017

Member

Changed the filter to a hashset CaptureTypes and updated the test case by @pchote.

Member

penev92 commented Nov 12, 2017

Changed the filter to a hashset CaptureTypes and updated the test case by @pchote.

@pchote

pchote approved these changes Nov 12, 2017

@pchote pchote merged commit ef878c6 into OpenRA:bleed Nov 12, 2017

2 checks passed

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

@forcecore forcecore deleted the forcecore:TransformOnCaptureFilter branch Jan 2, 2018

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