Skip to content
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

Added HpPerStep to Repairable trait and using this value in Repair activity #15111

Merged
merged 1 commit into from May 14, 2018

Conversation

Projects
None yet
5 participants
@teinarss
Copy link
Contributor

teinarss commented May 3, 2018

In the current form the repair activity is lacking the ability to set the HpPerStep for each unit with the Repairable trait. By only having this property on the RepairsUnits trait it will be hard to find a good value that works across all the repairable units in games.

For example in TD with the current HpPerStep in the live, repair time comparing to the build time on the units:

Unit Repair time (s) Build time (s)
Buggy 6 8
Bike 6 12
Medium tank 28 20
Mammoth tank 43 36

By have this be set on the trait Repairable it will enable mods to balance the repair activity in a better way.

Ping @AoAGeneral

@AoAGeneral

This comment has been minimized.

Copy link
Contributor

AoAGeneral commented May 3, 2018

Its been a bit of a debate back and forth on repair speeds currently. Just not obtainable due to buggies, bikes, hummers, and APCs repairing way to fast if adjusted otherwise. If this could be spread even across a bit more accurately (if im thinking this is what it does) going off base HP it could help make those adjustments. (So long as Mammoth tanks don't repair in 5 seconds anyways lol)

@@ -97,7 +99,7 @@ public override Activity Tick(Actor self)
if (remainingTicks == 0)
{
var unitCost = self.Info.TraitInfo<ValuedInfo>().Cost;
var hpToRepair = repairsUnits.Info.HpPerStep;
var hpToRepair = repairable != null ? repairable.Info.HpPerStep : repairsUnits.Info.HpPerStep;

This comment has been minimized.

@GraionDilach

GraionDilach May 3, 2018

Contributor

This doesn't work as intended - IIRC Repairable is the trait whichcan trigger this activity, so there is a really small chance you will reach the else path. Either way, you will need a special value (< 0 perhaps) to trigger the fallback, in which case you can even use that special value (0 on the trait) as the default to not trigger any explicit changes in the games without explicit modder/player intervention.

This comment has been minimized.

@GraionDilach

GraionDilach May 3, 2018

Contributor

You can then mention this fallback as a Desc() in your own HpPerStep.

This comment has been minimized.

@teinarss

teinarss May 3, 2018

Author Contributor

I was thinking of using a nullable int for the HpPerStep on the Repairable trait, Would it be better to use < 0 instead? I couldnt find any traits using nullable value types for properties in the code.

I also just saw that the helipad repair functionality doesn't use the Repair trait at all so I will need to fix handle that.

Thanks!

This comment has been minimized.

@GraionDilach

GraionDilach May 3, 2018

Contributor

Notsure if you can unset a value properly from the yaml if you're overriding something from inheritance + nullable with a default value is already considered set. Nullable just doesn't worth itself in this environment IMO.

TBH, this needs more thought in general, because I'm notsure if cost should also scale along with the per-unit repair value.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Can you squash the commits, please?

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented May 7, 2018

Just a question, am I suppose to do something more? :)

@@ -26,19 +26,22 @@ class RepairableInfo : ITraitInfo, Requires<HealthInfo>, Requires<IMoveInfo>

[VoiceReference] public readonly string Voice = "Action";

[Desc("The amount the unit will be repaired at each step. Use -1 for fallback behavior were HpPerStep from RepairUnit trait will be used.")]

This comment has been minimized.

@reaperrr

reaperrr May 7, 2018

Contributor

Minor typo nit: "where" instead of "were".

readonly Health health;
readonly IMove movement;
readonly AmmoPool[] ammoPools;

public Repairable(Actor self, RepairableInfo info)
{
this.info = info;
this.Info = info;

This comment has been minimized.

@reaperrr

reaperrr May 7, 2018

Contributor

style nit: you can remove the this. now.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 7, 2018

Just a question, am I suppose to do something more? :)

While you fix & squash the 2 minor nits I pointed out, please remove the "squash!" from the commit name as well.

For future reference, we usually comment with 'updated' when we've updated a PR so the reviewers notice it more quickly.

@teinarss teinarss force-pushed the teinarss:td_balancing_changes branch from f3d0de0 to 5942730 May 7, 2018

@teinarss teinarss force-pushed the teinarss:td_balancing_changes branch from 5942730 to c8c37be May 7, 2018

@teinarss

This comment has been minimized.

Copy link
Contributor Author

teinarss commented May 7, 2018

Updated: fixed the two minor issues!

Thanks for the clarifications.

@reaperrr reaperrr merged commit dcc11c7 into OpenRA:bleed May 14, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.