Page MenuHomeWildfire Games

StatusEffects v3 - More cleanup, allow stackable, upgrades.
Needs ReviewPublic

Authored by Freagarach on Sep 16 2019, 3:03 PM.

Details

Reviewers
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Follow up of D2281.


Done:

  • Allow techs to modify properties.
  • Allow stackable.
    • Ignore: Nothing happens really, the previous effect stays in effect.
    • Extend: The duration of the new status effect is added to the previous.
    • Replace: The previous effect is removed and the new effect is applied.
    • Stack: The new effect gets a new identifier and coexists next to the previous effect.
  • Fix a GUI issue where more than 5 icons would error out.
  • Added test for modification.

Possible extension:

  • Resistance.
Test Plan

Verify that:

  • the test techs apply (add more as desired).
  • the test status effects are correctly (re)applied as intended.
  • when applying more than 5 status effects there is no error shown, which was the case before this patch.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Do while.
  • statusIcons.length.
Vulcan added a comment.Tue, Feb 4, 9:32 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
|  71| »   »   »   »   temp·=·statusName·+·"_"·+·++i;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1711/display/redirect

Angen added a comment.Wed, Feb 5, 6:45 PM

(will do full review when resistance is done so some minor comments)

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
64

this can go without { }

binaries/data/mods/public/simulation/helpers/Attacking.js
133

let someVariable = template.ApplyStatus[effect].Modifiers[modifier] and use it in next lines so it become a bit cleaner or try to for of

Freagarach updated this revision to Diff 11294.Fri, Feb 7, 9:18 PM
Freagarach marked 2 inline comments as done.
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)
  • Remove unnecessary braces.
  • Use variable for readability in Attacking.js.
  • Added test for modifications.
Vulcan added a comment.Fri, Feb 7, 9:25 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
|  69| »   »   »   »   temp·=·statusName·+·"_"·+·++i;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1725/display/redirect

Vulcan added a comment.Fri, Feb 7, 9:29 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
|  69| »   »   »   »   temp·=·statusName·+·"_"·+·++i;
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1726/display/redirect

Angen added inline comments.Sat, Feb 8, 1:38 PM
binaries/data/mods/public/gui/session/selection_details.js
112

just note: == would be enough here, but probably better to have that extra security

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
69

you probably want i++, so _0 is considered as first option

binaries/data/mods/public/simulation/helpers/Attacking.js
126

1000 looks here like random number, StatusEffectReceiver has DefaultInterval property, that one should be used else this need to be kept in sync with it what is not good

also, does this mean that Interval will always exist ? making

 // With neither an interval nor a duration, there is no point in starting a timer.
	if (!status.Duration && !status.Interval)
		return;

and

// We want an interval to update the GUI to show how much time of the status effect
	// is left even if the status effect itself has no interval.
	if (!status.Interval)
		status._interval = this.DefaultInterval;

useless ?

140

what if Add == 0 and author wants to increase it later with techs ?

141

|| 0 is not needed because if check above, so if here, that do exist

143

same

Freagarach updated this revision to Diff 11301.Sun, Feb 9, 7:54 AM
Freagarach marked 6 inline comments as done.
  • i++.
  • No 1000 interval per se.
  • Allow people to start with 0 modification and increase it afterwards.
binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
69

Actually only the second SE gets this postfix, hence I chose ++i, but I guess it doesn't matter and makes the linter happy :)

Vulcan added a comment.Sun, Feb 9, 8:07 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1730/display/redirect

Angen added inline comments.Sun, Feb 9, 9:27 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
141

correctly should be 1 to make it mathematicly neutral

Freagarach updated this revision to Diff 11306.Sun, Feb 9, 1:52 PM
Freagarach marked an inline comment as done.

Do not use 0 as default for multiplication.

Vulcan added a comment.Sun, Feb 9, 1:57 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1733/display/redirect

Angen requested changes to this revision.Wed, Feb 12, 10:09 PM
Angen added a subscriber: Nescio.

applying status does not work, soldier was supposed to have +20 armour and still had basic stats

binaries/data/mods/public/simulation/helpers/Attacking.js
29

Defines how this status ..., currently it starts like question :)
\"Ignore\" means, that nothing happens (and so on)

@Nescio thoughts ?

This revision now requires changes to proceed.Wed, Feb 12, 10:09 PM
Nescio added inline comments.Thu, Feb 13, 9:46 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
29

Auras can be stackable too; how is it done there?
Also, you can use proper opening and closing quotation marks “ and ”, instead of escaping the straight programmer's quotes \".

Angen added inline comments.Thu, Feb 13, 9:54 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
29

thats defined in json so not documented in component or elsewhere

Nescio added inline comments.Thu, Feb 13, 9:56 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
29

Is this string something that shows up in game and is translated?

Angen added inline comments.Thu, Feb 13, 9:57 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
29

no, this is just for dev documentation, maybe some ide would show that, dont know

Nescio added inline comments.Thu, Feb 13, 10:08 AM
binaries/data/mods/public/simulation/helpers/Attacking.js
9

For comparison.
(Also full stop is missing.)

29

Then the exact formulation isn't terribly important. For comparison, simulation/components/Market.js has line 4:

	"<element name='TradeType' a:help='Specifies the type of possible trade route (land or naval).'>" +

Is there a default choice? Is this element optional?
Wouldn't simply False or True be more logical, though?

Freagarach updated this revision to Diff 11340.Thu, Feb 13, 7:47 PM
Freagarach marked 6 inline comments as done.
  • Extended tests.
  • Fixed additions not applying due to presence of multiplication.
binaries/data/mods/public/simulation/helpers/Attacking.js
29

True/False is not really an options, since there are four, mutually exclusive, choices.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1751/display/redirect

Freagarach updated this revision to Diff 11353.Fri, Feb 14, 7:37 PM

Explicitly test for undefined in calculations in Attacking.js.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1760/display/redirect

Angen added a comment.Wed, Feb 19, 6:40 PM

I had an idea.
Sorry but here it is.
Would it not be better if stack would increase values of current status effect instead add another?

Aside from this question everything works :)

Stan added a subscriber: Stan.Wed, Feb 19, 8:58 PM

Wouldn't work with different intervals?

Angen added a comment.Thu, Feb 20, 7:58 AM

I am not sure what do you mean.
I am coming with this idea because it could show in gui one icon with instead 4 when stacking up effect, but numbers in tooltip would need to be or generated or displayed how many times it is stacked so maybe not so good idea,

Stan added a comment.Thu, Feb 20, 8:06 AM

Ah I thought you meant stacking status effects not their icons. In the former case if status effects have different interval you cant combine them.

Angen accepted this revision.Thu, Feb 20, 8:10 AM

Confirmed by manual testing and code review and extended tests , that functionality of the summary is achieved.
I could not find any more flaws in code and design that I have reported already and have been addressed.
This is solving Todo from rP23448 so accepting.

This revision is now accepted and ready to land.Thu, Feb 20, 8:10 AM
Angen added a comment.Thu, Feb 20, 8:11 AM
In D2296#110758, @Stan wrote:

Ah I thought you meant stacking status effects not their icons. In the former case if status effects have different interval you cant combine them.

No I meant that originaly but now I see more problems with that :)

Stan added a comment.Thu, Feb 20, 8:17 AM

Getting on step closer to D1718.

elexis added a subscriber: elexis.Thu, Feb 20, 3:14 PM

The bogus template is only for testing, not to be committed?

binaries/data/mods/public/gui/common/tooltips.js
546–547

This translation is broken translate("%(statusName)s: " + foo)

550

tooltipAttributes.push("%(stackability)s") you probably mean to use translation for the string, otherwise there is probably no use case for using the sprintf format.

binaries/data/mods/public/simulation/components/tests/test_StatusEffectsReceiver.js
369

\n

Freagarach planned changes to this revision.Thu, Feb 20, 7:09 PM
Freagarach marked an inline comment as done.

Remove template (and upgrade) changes, fix translation code.

Freagarach updated this revision to Diff 11401.Thu, Feb 20, 8:56 PM
Freagarach marked 3 inline comments as done.
  • Refactor test.
  • Updated translation.

I will remove the template when the translation is approved.

This revision is now accepted and ready to land.Thu, Feb 20, 8:56 PM
Freagarach requested review of this revision.Thu, Feb 20, 8:56 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1795/display/redirect

Freagarach updated this revision to Diff 11410.Fri, Feb 21, 6:49 PM

Translation.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 401| 401| 			"durName": headerFont(translate("Duration")),
| 402| 402| 			"duration": getSecondsString((template._timeElapsed ?
| 403| 403| 				+template.Duration - template._timeElapsed :
| 404|    |-					+template.Duration) / 1000)
|    | 404|+				+template.Duration) / 1000)
| 405| 405| 		}));
| 406| 406| 	}
| 407| 407| 
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1802/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1804/display/redirect

Freagarach updated this revision to Diff 11413.Fri, Feb 21, 8:57 PM

Forgot one line.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1805/display/redirect

bb added a subscriber: bb.Fri, Feb 21, 10:52 PM

Random thought for the if else repeat mess: put all strings in a global with MarkForTranslation and get the correct on by constructing it using the template properties. Also translators might like some context

Nescio removed a subscriber: Nescio.Sun, Feb 23, 10:19 AM