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

Change LowPowerSlowdown to LowPowerModifier #14664

Merged
merged 4 commits into from Oct 8, 2018

Conversation

Projects
None yet
6 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Jan 4, 2018

Also removed LowPowerSlowdown:s that were already equal to default value.

Didn't write a upgrade rule, not sure if wanted, as we are gonna rewrite them anyway.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 29ec0f3 to 8873dbc Jan 4, 2018

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jan 4, 2018

This, along with ProductionQueue->BuildDurationModifier should probably be ripped out to a separate conditional QueueSpeedModifier traits by this point.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 8873dbc to ef4033a Jan 13, 2018

@@ -508,7 +508,7 @@ public void Tick(PlayerResources pr)
if (pm.PowerState != PowerState.Normal)
{
if (--Slowdown <= 0)
Slowdown = Queue.Info.LowPowerSlowdown;
Slowdown = Queue.Info.LowPowerModifier;

This comment has been minimized.

@alercah

alercah Jan 13, 2018

Contributor

This means that the slowdown is now multiplied by 100. Every tick, the Slowdown will go down by 1, so it will now take 300 ticks to do one build tick on low power. Allowing modifiers that aren't effectively multiple of 100 will require careful work to allow for fractional build ticks. I think it may be possible to do by replacing --Slowdown with Slowdown -= 100, but I'm not 100% sure.

You asked on IRC about the interaction with #14702; I have nothing in principle against allowing a modifier to speed up the build queue (values less than 100). That would require the logic here to allow the build to tick multiple times per game tick.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from ef4033a to 8666bcf Jan 14, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Jan 14, 2018

Updated, if my calculations are correct, this should do the job. I did a debug message locally with 175 modifier on Armor queue, values looks correct.

openra-2018-01-14t045339580z

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 8666bcf to 28f6d3c Jan 14, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Feb 23, 2018

Didn't write a upgrade rule, not sure if wanted, as we are gonna rewrite them anyway.

There's currently no ETA for this, and the upgrade rule for this PR should be simple (rename and multiply value by 100).

Also needs a rebase.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 28f6d3c to fd62250 Feb 24, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Feb 24, 2018

Rebased and added a upgrade rule.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me otherwise. Needs a rebase though.

@@ -508,8 +508,8 @@ public void Tick(PlayerResources pr)

if (pm.PowerState != PowerState.Normal)
{
if (--Slowdown <= 0)
Slowdown = Queue.Info.LowPowerSlowdown;
if ((Slowdown -= 100) <= 0)

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 19, 2018

Member

This looks weird, why didn't you use just if (Slowdown - 100 <= 0)?

This comment has been minimized.

@MustaphaTR

MustaphaTR Mar 31, 2018

Member

Sorry, i haven't answered, because i wasn't sure. But looks like -= is correct. Slowdown needs to tick down, otherwise unit won't be built.

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It would be cleaner to split this onto two lines:

Slowdown -= 100;
if (SlowDown < 0)
    ...

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch 2 times, most recently from 1e35557 to 31cf5c0 Mar 31, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Mar 31, 2018

Rebased. Moved the update rule to the new system, but did not test it.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Apr 17, 2018

NTM: Looks like this needs rebase again.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 20, 2018

Ping @MustaphaTR.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 31cf5c0 to 1044564 May 21, 2018

@pchote
Copy link
Member

pchote left a comment

A few thoughts:

@@ -80,8 +80,8 @@ public class ProductionQueueInfo : ITraitInfo

public void RulesetLoaded(Ruleset rules, ActorInfo ai)
{
if (LowPowerSlowdown <= 0)
throw new YamlException("Production queue must have LowPowerSlowdown of at least 1.");
if (LowPowerModifier < 100)

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

The previous check was needed because 0 or negative values would break the calculation. IMO there is no reason not to disallow modifiers between 1 and 100 (implying a speed up on low power) here.

@@ -508,8 +508,8 @@ public void Tick(PlayerResources pr)

if (pm.PowerState != PowerState.Normal)
{
if (--Slowdown <= 0)
Slowdown = Queue.Info.LowPowerSlowdown;
if ((Slowdown -= 100) <= 0)

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It would be cleaner to split this onto two lines:

Slowdown -= 100;
if (SlowDown < 0)
    ...
[Desc("The build time is multiplied with this value on low power.")]
public readonly int LowPowerSlowdown = 3;
[Desc("The build time is multiplied with this percentage on low power.")]
public readonly int LowPowerModifier = 300;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

It may make sense to take this chance to reset the default to 100? a 3x slowdown is quite arbitrary, and specific to the default C&C mods.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch 2 times, most recently from b25c03c to 7697753 Sep 29, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Sep 29, 2018

Updated.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 7697753 to ea81202 Oct 5, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Oct 5, 2018

#15018 is merged. There was no merge conflict, but i still rebased to the lastest bleed. I haven't checked if #15018 added anything i should have edited for this to properly work tho.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 6, 2018

The build palette thinks it takes over an hour to build an APC in low power:

screenshot 2018-10-06 at 17 14 59

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 6, 2018

I suspect the issue is that ProductionItem.RemainingTimeActual is missing a division by 100.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from ea81202 to 089868a Oct 6, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Oct 6, 2018

Yeah, that was the issue. Updated.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good to me now. Needs a rebase however.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:low-power-modifier branch from 089868a to fbf91ea Oct 7, 2018

@pchote pchote removed the PR: Rebase me! label Oct 8, 2018

@pchote
Copy link
Member

pchote left a comment

LGTM and looks fine ingame.

Just one tidy in the update rule, which I will push myself shortly to save you from yet another update cycle.

foreach (var q in actorNode.ChildrenMatching(trait))
queues.Add(q);

foreach (var queue in queues)

This comment has been minimized.

@pchote

pchote Oct 8, 2018

Member

This can simplify to

foreach (var queue in queueTraits.SelectMany(q => actorNode.ChildrenMatching(q)))

and then the code above removed.

@pchote pchote force-pushed the MustaphaTR:low-power-modifier branch from fbf91ea to 9084209 Oct 8, 2018

@pchote

pchote approved these changes Oct 8, 2018

@pchote pchote merged commit 47c4be9 into OpenRA:bleed Oct 8, 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:low-power-modifier branch Oct 9, 2018

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