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

Grant a condition while performing an external capture #13854

Merged
merged 2 commits into from Sep 14, 2017

Conversation

Projects
None yet
7 participants
@gwenzek
Contributor

gwenzek commented Aug 15, 2017

Add a notifier for the beginning of an external capture
Implement uncloaking when a capture begins (#13767)

The attached replay demonstrate engineers modded with the cloaking trait that get uncloaked when they starts capturing an enemy building.
OpenRA-2017-08-15T165036Z.orarep.zip

I'm not sure what will happen if the cloaked unit finish capturing the building.
Should it get cloaked again ? Does that happen automatically ? Could that happen while the unit is still capturing the building ?

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Aug 15, 2017

Member

Travis isn't happy:

OpenRA.Mods.Common/Activities/ExternalCaptureActor.cs:L16: [SA1208] System using directives must be placed before all other using directives.

You should move using System.Linq above other usings.

Member

MustaphaTR commented Aug 15, 2017

Travis isn't happy:

OpenRA.Mods.Common/Activities/ExternalCaptureActor.cs:L16: [SA1208] System using directives must be placed before all other using directives.

You should move using System.Linq above other usings.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Aug 15, 2017

Member

Engineer is cloaking back while it is still capturing with this PR. I checked Generals to be sure, at there Infantries are uncloaked during the whole capturing.

Member

MustaphaTR commented Aug 15, 2017

Engineer is cloaking back while it is still capturing with this PR. I checked Generals to be sure, at there Infantries are uncloaked during the whole capturing.

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Aug 26, 2017

Contributor

@MustaphaTR thanks for the review.
I changed so the unit doesn't cloak while capturing.
I added a "OnCaptureCancelled" event to detect when the capture is cancelled so the unit will cloak back.
I haven't see other "ActionCancelled" events so I'm not sure that's the correct pattern.

Contributor

gwenzek commented Aug 26, 2017

@MustaphaTR thanks for the review.
I changed so the unit doesn't cloak while capturing.
I added a "OnCaptureCancelled" event to detect when the capture is cancelled so the unit will cloak back.
I haven't see other "ActionCancelled" events so I'm not sure that's the correct pattern.

@MustaphaTR

👍

@SoScared

This comment has been minimized.

Show comment
Hide comment
@SoScared

SoScared Sep 2, 2017

Member

Just to be clear, does this add a notifier with engineer capturing in RA?

Member

SoScared commented Sep 2, 2017

Just to be clear, does this add a notifier with engineer capturing in RA?

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 2, 2017

Member

No. This adds a Capturing, cloak RevealOn type, so i can make my GLA Rebels with Camouflage reveal while capturing on my mod.

Member

MustaphaTR commented Sep 2, 2017

No. This adds a Capturing, cloak RevealOn type, so i can make my GLA Rebels with Camouflage reveal while capturing on my mod.

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Sep 2, 2017

Contributor

@SoScared Yes my implementation adds a notifier for engineer capturing in RA that's how I tested it.
The traits Cloak listen for this notifier and uncloack the unit when capturing.

Contributor

gwenzek commented Sep 2, 2017

@SoScared Yes my implementation adds a notifier for engineer capturing in RA that's how I tested it.
The traits Cloak listen for this notifier and uncloack the unit when capturing.

@Smittytron

This comment has been minimized.

Show comment
Hide comment
@Smittytron

Smittytron Sep 2, 2017

Contributor

Yes my implementation adds a notifier for engineer capturing in RA that's how I tested it.
The traits Cloak listen for this notifier and uncloack the unit when capturing.

But not a notification that players hear right? Asking because a few weeks ago someone was suggesting that you should be notified when your building is being captured. This suggestion is universally opposed by the competitive community.

Contributor

Smittytron commented Sep 2, 2017

Yes my implementation adds a notifier for engineer capturing in RA that's how I tested it.
The traits Cloak listen for this notifier and uncloack the unit when capturing.

But not a notification that players hear right? Asking because a few weeks ago someone was suggesting that you should be notified when your building is being captured. This suggestion is universally opposed by the competitive community.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 2, 2017

Member

No, this is not a notification. A notifier and notification is different things. I think notification was what SoScared meant too, thats why i answered no. A notifier allows stuff a trait does to be used by another trait, in this case making ExternalCaptures uncloak a unit.

Member

MustaphaTR commented Sep 2, 2017

No, this is not a notification. A notifier and notification is different things. I think notification was what SoScared meant too, thats why i answered no. A notifier allows stuff a trait does to be used by another trait, in this case making ExternalCaptures uncloak a unit.

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Sep 5, 2017

Contributor

Thanks @MustaphaTR for the clarifications. I didn't properly understand SoScared concerns.

@pchote Thanks for your suggestions. This is my first PR for OpenRA and wasn't aware of this Condition granting mechanism. It look interesting. I just have small concerns about using it here.

Currently the YAML for a cloaking unit looks like:

	Cloak:
		InitialDelay: 250
		CloakDelay: 120
		UncloakOn: Attack, Unload, Infiltrate, Demolish
		CloakTypes: Cloak
		IsPlayerPalette: true

My PR would allow to just add "Capturing" to the "UncloakOn" property list.
Using a condition, it will look something like:

	Cloak:
		InitialDelay: 250
		CloakDelay: 120
		UncloakOn: Attack, Unload, Infiltrate, Demolish
		CloakTypes: Cloak
		IsPlayerPalette: true
		RequiresCondition: !capturing

Don't you find that a bit awkward to have two separate settings for controlling the Uncloaking ?

Contributor

gwenzek commented Sep 5, 2017

Thanks @MustaphaTR for the clarifications. I didn't properly understand SoScared concerns.

@pchote Thanks for your suggestions. This is my first PR for OpenRA and wasn't aware of this Condition granting mechanism. It look interesting. I just have small concerns about using it here.

Currently the YAML for a cloaking unit looks like:

	Cloak:
		InitialDelay: 250
		CloakDelay: 120
		UncloakOn: Attack, Unload, Infiltrate, Demolish
		CloakTypes: Cloak
		IsPlayerPalette: true

My PR would allow to just add "Capturing" to the "UncloakOn" property list.
Using a condition, it will look something like:

	Cloak:
		InitialDelay: 250
		CloakDelay: 120
		UncloakOn: Attack, Unload, Infiltrate, Demolish
		CloakTypes: Cloak
		IsPlayerPalette: true
		RequiresCondition: !capturing

Don't you find that a bit awkward to have two separate settings for controlling the Uncloaking ?

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Sep 5, 2017

Contributor

I've started an implementation using the Trait Conditions.
The code is much simpler.
If you agree that it's not a problem to use Trait Conditions while other uncloaking events happen in another fashion, I'll create a new PR.
Meanwhile it's available here: bleed...gwenzek:uncloak_on_capture

Contributor

gwenzek commented Sep 5, 2017

I've started an implementation using the Trait Conditions.
The code is much simpler.
If you agree that it's not a problem to use Trait Conditions while other uncloaking events happen in another fashion, I'll create a new PR.
Meanwhile it's available here: bleed...gwenzek:uncloak_on_capture

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 5, 2017

Member

Don't you find that a bit awkward to have two separate settings for controlling the Uncloaking ?

Nope, we already do exactly this to disable cloak on critical health for all our current stealth units.
See this example from the TD stealth tank:

Cloak:
InitialDelay: 90
CloakDelay: 90
CloakSound: trans1.aud
UncloakSound: trans1.aud
RequiresCondition: !cloak-force-disabled
GrantConditionOnDamageState@UNCLOAK:
Condition: cloak-force-disabled
ValidDamageStates: Critical

Member

pchote commented Sep 5, 2017

Don't you find that a bit awkward to have two separate settings for controlling the Uncloaking ?

Nope, we already do exactly this to disable cloak on critical health for all our current stealth units.
See this example from the TD stealth tank:

Cloak:
InitialDelay: 90
CloakDelay: 90
CloakSound: trans1.aud
UncloakSound: trans1.aud
RequiresCondition: !cloak-force-disabled
GrantConditionOnDamageState@UNCLOAK:
Condition: cloak-force-disabled
ValidDamageStates: Critical

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 5, 2017

Member

Currently disabling capturing while unit is damaged to red is handled by conditions too. I think it would be better to use conditions, One may find other uses of this condition granting than disabling cloak.

Member

MustaphaTR commented Sep 5, 2017

Currently disabling capturing while unit is damaged to red is handled by conditions too. I think it would be better to use conditions, One may find other uses of this condition granting than disabling cloak.

@gwenzek gwenzek changed the title from add a notifier for the beginning of an external capture to Grant a condition while performing an external capture Sep 5, 2017

@pchote

Looking good overall. Just two issues, then I expect to 👍.

Show outdated Hide outdated OpenRA.Mods.Common/Activities/ExternalCaptureActor.cs Outdated
if (capturable.CaptureInProgress)
capturable.EndCapture();
EndCapture(self);

This comment has been minimized.

@pchote

pchote Sep 9, 2017

Member

You need to call this from the case above too, otherwise the condition will remain permanently granted if the target actor dies while being captured.

@pchote

pchote Sep 9, 2017

Member

You need to call this from the case above too, otherwise the condition will remain permanently granted if the target actor dies while being captured.

This comment has been minimized.

@gwenzek

gwenzek Sep 9, 2017

Contributor

good catch ! Done

@gwenzek

gwenzek Sep 9, 2017

Contributor

good catch ! Done

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 9, 2017

Member

@MustaphaTR: This has changed a lot since your original 👍, so can you please check this again once the fixups are done?

Member

pchote commented Sep 9, 2017

@MustaphaTR: This has changed a lot since your original 👍, so can you please check this again once the fixups are done?

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Sep 11, 2017

Contributor

Fixup done.

Also what's the best way to notify the reviewers I fixed the issue they spotted and they can re-review the code ? Do I need to write in this thread or just answering in the diff view works ?

Contributor

gwenzek commented Sep 11, 2017

Fixup done.

Also what's the best way to notify the reviewers I fixed the issue they spotted and they can re-review the code ? Do I need to write in this thread or just answering in the diff view works ?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Sep 11, 2017

Member

Answering in the diff view works, but it can sometimes (unfortunately with our current lack of reviewing bandwidth, that has regressed to "usually") take a while for someone to come back to it.

Sorry! and thanks for the reminder - I'll try and recheck this tonight.

Member

pchote commented Sep 11, 2017

Answering in the diff view works, but it can sometimes (unfortunately with our current lack of reviewing bandwidth, that has regressed to "usually") take a while for someone to come back to it.

Sorry! and thanks for the reminder - I'll try and recheck this tonight.

@pchote

👍 after one last bug fix:

Show outdated Hide outdated OpenRA.Mods.Common/Activities/ExternalCaptureActor.cs Outdated

@pchote pchote added the PR: Needs +2 label Sep 13, 2017

@abcdefg30 abcdefg30 merged commit 11a990e into OpenRA:bleed Sep 14, 2017

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.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Sep 14, 2017

@gwenzek

This comment has been minimized.

Show comment
Hide comment
@gwenzek

gwenzek Sep 14, 2017

Contributor

Thanks everyone for the review!
This was my first PR on OpenRA so I needed a lot of hand holding, thanks for your patience!
Cheers.

Contributor

gwenzek commented Sep 14, 2017

Thanks everyone for the review!
This was my first PR on OpenRA so I needed a lot of hand holding, thanks for your patience!
Cheers.

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