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 GrantConditionOnProduction #15303

Merged
merged 1 commit into from Sep 29, 2018

Conversation

Projects
None yet
6 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Jun 28, 2018

This is used for actor specific upgrades (Overlord Tank/Helix) on my mod. Decided to PR upstream as it can be useful for other modders.

public object Create(ActorInitializer init) { return new GrantConditionOnProduction(init.Self, this); }
}

public class GrantConditionOnProduction : INotifyCreated, INotifyOtherProduction

This comment has been minimized.

@pchote

pchote Jun 28, 2018

Member

Why not INotifyProduction?

This comment has been minimized.

@MustaphaTR

MustaphaTR Jun 28, 2018

Member

I didn't realise we already had such interface, yeah probably a better idea and actually is what i was looking for.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:grant-condition-on-production branch from d482600 to e50318e Jun 28, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jun 28, 2018

Updated.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 26, 2018

IMO it should get a duration.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:grant-condition-on-production branch 3 times, most recently from aa11d1e to e788704 Jul 28, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jul 28, 2018

Updated with Graion's request.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jul 28, 2018

You don't reset the duration if the condition is already applied.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:grant-condition-on-production branch from e788704 to b72fb0c Jul 28, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jul 28, 2018

Updated again.


void INotifyProduction.UnitProduced(Actor self, Actor other, CPos exit)
{
if (!info.Actors.Any() || info.Actors.Select(a => a.ToLowerInvariant()).Contains(other.Info.Name))

This comment has been minimized.

@abcdefg30

abcdefg30 Jul 28, 2018

Member

This statement should get braces, or be transformed into a "return early" version.

Show resolved Hide resolved OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnProduction.cs Outdated

@chrisforbes chrisforbes added this to the Next + 1 milestone Jul 29, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:grant-condition-on-production branch from b72fb0c to 5154316 Jul 30, 2018

@Mailaender
Copy link
Member

Mailaender left a comment

Looks good.

@pchote
Copy link
Member

pchote left a comment

Works as advertised, but a couple of style issues to fix before merging:

public class GrantConditionOnProduction : INotifyCreated, INotifyProduction, ITick, ISync, ISelectionBar
{
readonly GrantConditionOnProductionInfo info;
ConditionManager manager;

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Please call this conditionManager for consistency with other traits.

if (info.Actors.Any() && !info.Actors.Select(a => a.ToLowerInvariant()).Contains(other.Info.Name))
return;

if (token == ConditionManager.InvalidConditionToken)

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Please change this to

if (conditionManager != null && token == ConditionManager.InvalidConditionToken)
	token = manager.GrantCondition(self, cond);

We don't gain anything by splitting out a separate method.

if (--ticks < 0)
{
ticks = info.Duration;
RevokeCondition(self);

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Likewise here, this should change to token = manager.RevokeCondition(self, token);.
An extra check for the condition manager is unnecessary because this is built in to token != ConditionManager.InvalidConditionToken.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:grant-condition-on-production branch from 5154316 to 926d320 Sep 29, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Sep 29, 2018

Updated.

@pchote

pchote approved these changes Sep 29, 2018

@pchote pchote merged commit b53c13d into OpenRA:bleed Sep 29, 2018

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:grant-condition-on-production branch Sep 29, 2018

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