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

Add DisguisedAsCondition to Disguise #14059

Merged
merged 2 commits into from Nov 15, 2017

Conversation

Projects
None yet
5 participants
@MustaphaTR
Member

MustaphaTR commented Sep 20, 2017

First commit fixes a part of #13765.

Second commit adds DisguisedAsCondition to Disguise. Similiar to what Cargo has, you can make Disguised actor get a condition depending on what it disguised as. In the future WithDisguisingInfantryBody can be scrapped and replaced by these conditions, i keep it for now tho and only add support.

I added a TESTCASE for second commit, it makes it so Spy's eye icon will only be shown for Rifle, Gren, Rocket or Tanya and not other infantries.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Oct 20, 2017

Member

Updated.

Member

MustaphaTR commented Oct 20, 2017

Updated.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Oct 22, 2017

Member

Dropped first commit, #14236 does it in a better way.

Member

MustaphaTR commented Oct 22, 2017

Dropped first commit, #14236 does it in a better way.

@MustaphaTR MustaphaTR changed the title from Add GrantConditionByDisguisedActor and Unhardcode Disguise TargetType to Add DisguisedAsCondition to Disguise Oct 22, 2017

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 3, 2017

Member

Rebased.

Member

MustaphaTR commented Nov 3, 2017

Rebased.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 13, 2017

Member

Worth noting is that the special-case conditions are granted in addition to the generic one, not instead of it.
Not sure if this is intentional, but I want to hear some opinions on which would be more useful to modders before I merge this.

Otherwise 👍

Member

penev92 commented Nov 13, 2017

Worth noting is that the special-case conditions are granted in addition to the generic one, not instead of it.
Not sure if this is intentional, but I want to hear some opinions on which would be more useful to modders before I merge this.

Otherwise 👍

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 13, 2017

Member

It is intented that there are both specific ad generic disguised condition. That is the case for Cargo: too. You probably wouldn't want to list all the units with ||s to make a condition given by all units.

Generic disguised condition is removed on TESTCASE for RA spy because travis was not happy that it goes unused.

I guess this is OK, now. Should i remove the testcase?

Member

MustaphaTR commented Nov 13, 2017

It is intented that there are both specific ad generic disguised condition. That is the case for Cargo: too. You probably wouldn't want to list all the units with ||s to make a condition given by all units.

Generic disguised condition is removed on TESTCASE for RA spy because travis was not happy that it goes unused.

I guess this is OK, now. Should i remove the testcase?

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 13, 2017

Contributor

Agreeing completely with @MustaphaTR on the reasoning behind the condition setup.

Contributor

GraionDilach commented Nov 13, 2017

Agreeing completely with @MustaphaTR on the reasoning behind the condition setup.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 13, 2017

Member

OK then. Yes, remove the testcase.

Member

penev92 commented Nov 13, 2017

OK then. Yes, remove the testcase.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 13, 2017

Member

I'd still like to merge the DisguisedAsConditions and DisguisedCondition into a single special cased field as I explained when this topic first came up in IRC. I will write some code to demonstrate how to do this - please remind me if I haven't followed up on this within a couple of days.

Member

pchote commented Nov 13, 2017

I'd still like to merge the DisguisedAsConditions and DisguisedCondition into a single special cased field as I explained when this topic first came up in IRC. I will write some code to demonstrate how to do this - please remind me if I haven't followed up on this within a couple of days.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 15, 2017

Member

Playing with this a bit, I my request above creates more trouble than it is worth, so we should forget about that.

Instead, i'd like to request that you merge in a small scope creep that I have already written: c981885 does some small code style cleanups that should be fixup-merged into your first commit, and then 2249fdd removes the sprite handling from Disguise in preparation for eventually removing WithDisguisingInfantryBody completely.

👍 from me after those are included.

Member

pchote commented Nov 15, 2017

Playing with this a bit, I my request above creates more trouble than it is worth, so we should forget about that.

Instead, i'd like to request that you merge in a small scope creep that I have already written: c981885 does some small code style cleanups that should be fixup-merged into your first commit, and then 2249fdd removes the sprite handling from Disguise in preparation for eventually removing WithDisguisingInfantryBody completely.

👍 from me after those are included.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 15, 2017

Member

Updated as @pchote said. And removed the testcase.

Member

MustaphaTR commented Nov 15, 2017

Updated as @pchote said. And removed the testcase.

@pchote

pchote approved these changes Nov 15, 2017

@pchote pchote merged commit f35f6c0 into OpenRA:bleed Nov 15, 2017

2 checks passed

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

@MustaphaTR MustaphaTR deleted the MustaphaTR:disguise-condition-and-target-type branch Nov 15, 2017

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