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

Made Valued optional for traits who do not require it. #15367

Merged
merged 1 commit into from Sep 28, 2018

Conversation

Projects
None yet
6 participants
@IceReaper
Copy link
Contributor

IceReaper commented Jul 22, 2018

Some traits are hardcoded against Valued, but do not rely on them. This PR fixes a crash caused by these traits when an actor does not have a Valued trait. No more need to add Valued with Cost: 0 to actors to avoid the crashes.

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch 2 times, most recently from 80448a6 to 12aa302 Jul 22, 2018

@@ -98,7 +98,7 @@ public override Activity Tick(Actor self)

if (remainingTicks == 0)
{
var unitCost = self.Info.TraitInfo<ValuedInfo>().Cost;
var unitCost = self.Info.HasTraitInfo<ValuedInfo>() ? self.Info.TraitInfo<ValuedInfo>().Cost : 0;

This comment has been minimized.

@pchote

pchote Jul 25, 2018

Member

Use var valuedInfo = self.Info.TraitInfoOrDefault<ValuedInfo>() and then do a null check instead of performing two lookups!

This same comment applies to the other changes in the commit too.

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from 12aa302 to 810c165 Jul 25, 2018

@@ -60,7 +60,8 @@ public GainsExperience(ActorInitializer init, GainsExperienceInfo info)
this.info = info;

MaxLevel = info.Conditions.Count;
var cost = self.Info.TraitInfo<ValuedInfo>().Cost;
var valued = self.Info.TraitInfoOrDefault<ValuedInfo>();
var cost = valued != null ? valued.Cost : 0;
foreach (var kv in info.Conditions)
nextLevel.Add(Pair.New(kv.Key * cost, kv.Value));

This comment has been minimized.

@pchote

pchote Jul 26, 2018

Member

(this line, specifically).

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from 810c165 to 6f6841f Jul 26, 2018

@@ -60,9 +63,10 @@ public GainsExperience(ActorInitializer init, GainsExperienceInfo info)
this.info = info;

MaxLevel = info.Conditions.Count;
var cost = self.Info.TraitInfo<ValuedInfo>().Cost;
var valued = self.Info.TraitInfoOrDefault<ValuedInfo>();

This comment has been minimized.

@GraionDilach

GraionDilach Jul 26, 2018

Contributor

This needs to be shifted to INotifyCreated.Created.

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch 2 times, most recently from 3fa4cd8 to 84d8849 Jul 26, 2018

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch 2 times, most recently from c6ca14e to c4fa87a Jul 29, 2018

@pchote pchote dismissed their stale review Jul 29, 2018

Code updated.

@reaperrr
Copy link
Contributor

reaperrr left a comment

Code looks good to me, haven't tested it though.

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from c4fa87a to 6dd52b4 Aug 15, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Aug 15, 2018

Rebased.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from 6dd52b4 to 25ba9dc Aug 15, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise.


if (init.Contains<ExperienceInit>())
initialExperience = init.Get<ExperienceInit, int>();
}

void INotifyCreated.Created(Actor self)
{
var valued = self.Info.TraitInfoOrDefault<ValuedInfo>();
var requiredExperience = info.RequiredExperience == -1 ? (valued != null ? valued.Cost : 0) : info.RequiredExperience;

This comment has been minimized.

@abcdefg30

abcdefg30 Aug 16, 2018

Member

Would a negative RequiredExperience make sense at all? If not, please change this to info.RequiredExperience < 0.

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

@IceReaper: would you mind changing this too?

@@ -33,6 +33,9 @@ public class GainsExperienceInfo : ITraitInfo, Requires<ValuedInfo>
[Desc("Palette for the level up sprite.")]
[PaletteReference] public readonly string LevelUpPalette = "effect";

[Desc("Experience factor for each levelup. Will default to Valued Cost.")]
public readonly int RequiredExperience = -1;

This comment has been minimized.

@GraionDilach

GraionDilach Aug 16, 2018

Contributor

Considering #15529, it might be a better option to have an enum defined here to choose between absolute values for the levels, relative values to a base value and relative values to Valued->Cost.

This comment has been minimized.

@MustaphaTR

MustaphaTR Aug 16, 2018

Member

I think this approach is ok. RequiredExperience: 1 would give absolute value. Tho maybe ExperienceFactor would be a better name to indicate that it is a multiplier.

This comment has been minimized.

@GraionDilach

GraionDilach Aug 16, 2018

Contributor

Alright then.

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

@IceReaper: Can we please address these comments by renaming this to ExperienceModifier (for consistency with DamageMultiplier, FirepowerMultiplier, etc) and the description to "Multiplier to apply to the Conditions keys. Defaults to the actor's value." You will also need to change the description for Conditions above.

@pchote
Copy link
Member

pchote left a comment

LGTM, just reminding/commenting on a couple of the existing nits:

@@ -33,6 +33,9 @@ public class GainsExperienceInfo : ITraitInfo, Requires<ValuedInfo>
[Desc("Palette for the level up sprite.")]
[PaletteReference] public readonly string LevelUpPalette = "effect";

[Desc("Experience factor for each levelup. Will default to Valued Cost.")]
public readonly int RequiredExperience = -1;

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

@IceReaper: Can we please address these comments by renaming this to ExperienceModifier (for consistency with DamageMultiplier, FirepowerMultiplier, etc) and the description to "Multiplier to apply to the Conditions keys. Defaults to the actor's value." You will also need to change the description for Conditions above.


if (init.Contains<ExperienceInit>())
initialExperience = init.Get<ExperienceInit, int>();
}

void INotifyCreated.Created(Actor self)
{
var valued = self.Info.TraitInfoOrDefault<ValuedInfo>();
var requiredExperience = info.RequiredExperience == -1 ? (valued != null ? valued.Cost : 0) : info.RequiredExperience;

This comment has been minimized.

@pchote

pchote Sep 23, 2018

Member

@IceReaper: would you mind changing this too?

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from 25ba9dc to b457de4 Sep 26, 2018

@IceReaper

This comment has been minimized.

Copy link
Contributor

IceReaper commented Sep 26, 2018

Updated and rebased!

Code changed


if (init.Contains<ExperienceInit>())
initialExperience = init.Get<ExperienceInit, int>();
}

void INotifyCreated.Created(Actor self)
{
var valued = self.Info.TraitInfoOrDefault<ValuedInfo>();
var requiredExperience = info.ExperienceModifier < 0 ? (valued != null ? valued.Cost : 0) : info.ExperienceModifier;

This comment has been minimized.

@pchote

pchote Sep 27, 2018

Member

This needs to default to 1 if valued is null, otherwise all keys will reduce to 0!

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 28, 2018

Member

Is this not intentional? From the description: Defaults to the actor's value.. If the actor has no value, this defaults to no value as well.

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

The end result is that it breaks the trait. If this multiplier is 0 then all the experience levels defined in the yaml above are ignored and the unit will immediately rank up to its max level as soon as it receives any exp.

This comment has been minimized.

@pchote

pchote Sep 28, 2018

Member

Adding a RulesetLoaded check to throw an error if the modifier is -1 and Valued doesn't exist (similarly to how Armament handles invalid weapon references) would provide a way out of this.

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from b457de4 to f0456b9 Sep 28, 2018

@IceReaper IceReaper force-pushed the IceReaper:optional_valued branch from f0456b9 to 0384b43 Sep 28, 2018

@pchote

pchote approved these changes Sep 28, 2018

@pchote pchote merged commit a86f41c into OpenRA:bleed Sep 28, 2018

2 checks passed

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

@IceReaper IceReaper deleted the IceReaper:optional_valued branch Sep 28, 2018

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