Page MenuHomeWildfire Games

StatusEffects v3 - More cleanup, allow stackable, upgrades.
Changes PlannedPublic

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

Details

Reviewers
Angen
wraitii
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.
  • Allow to stack only a limited number of times.
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
Freagarach marked 6 inline comments as done.Feb 13 2020, 7:47 PM
  • 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.Feb 14 2020, 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.Feb 19 2020, 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.Feb 19 2020, 8:58 PM

Wouldn't work with different intervals?

Angen added a comment.Feb 20 2020, 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.Feb 20 2020, 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.Feb 20 2020, 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.Feb 20 2020, 8:10 AM
Angen added a comment.Feb 20 2020, 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.Feb 20 2020, 8:17 AM

Getting on step closer to D1718.

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

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

binaries/data/mods/public/gui/common/tooltips.js
398

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

401

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
577

\n

Freagarach planned changes to this revision.Feb 20 2020, 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.Feb 20 2020, 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.Feb 20 2020, 8:56 PM
Freagarach requested review of this revision.Feb 20 2020, 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.Feb 21 2020, 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.Feb 21 2020, 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.Feb 21 2020, 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.Feb 23 2020, 10:19 AM
Freagarach added a comment.EditedMar 6 2020, 8:42 PM

http://irclogs.wildfiregames.com/2020-03/2020-03-06-QuakeNet-%230ad-dev.log

<elexis> that was to put the strings into templates
<elexis> this wayyou dont have to specify all of them
<elexis> though not suer if right for gui
<elexis> I mean putting gui data into templates is usually not so nice, but its per entity data and there are similar precedents such as entity description

@bb thoughts?

bb added a comment.Mar 6 2020, 10:36 PM

What happens if these values become modifiable by techs? or otherwise get modified during the game? putting the strings in the template basically kills this possibility (one should also mod the tooltip with the same tech => hard to maintain). However I am not sure if we need to allow this to be modifiable.

<elexis> this wayyou dont have to specify all of them

well that way you specify them in the template, potentially several times

What could be done is to have %(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s %(stackability)s defined in template.
But duration and interval can be changed through aura or technology, what makes 4 options defined in template and 4 ifs in code what will make it easier to read.
Downside is 4 strings for every template.

I do not see how moving translate("%(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s") to global would help. One would need to develop some key generation. Possibly concatenating some strings based on template values. Plus could be translation is done only once, but key building could be more costly than current translate calls.

Now there are 5 if branching needed, what is one more compared to first option and the same with second one.
One if branch removal with 4 strings in templates what cost memory is not exactly good trade-off, unless we do not care about additional memory.
Second choice is more questionable if should or should not be used. One could compare time spend in the function with current and second approach and see which one is better.

Angen requested changes to this revision.Mar 23 2020, 4:16 PM

moving out of queue

This revision now requires changes to proceed.Mar 23 2020, 4:16 PM
Freagarach edited the summary of this revision. (Show Details)Apr 12 2020, 8:45 AM
elexis added a comment.Thu, May 7, 3:09 PM
In D2296#111529, @Angen wrote:

What could be done is to have %(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s %(stackability)s defined in template.
But duration and interval can be changed through aura or technology, what makes 4 options defined in template and 4 ifs in code what will make it easier to read.
Downside is 4 strings for every template.
I do not see how moving translate("%(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s") to global would help. One would need to develop some key generation. Possibly concatenating some strings based on template values. Plus could be translation is done only once, but key building could be more costly than current translate calls.
Now there are 5 if branching needed, what is one more compared to first option and the same with second one.
One if branch removal with 4 strings in templates what cost memory is not exactly good trade-off, unless we do not care about additional memory.
Second choice is more questionable if should or should not be used. One could compare time spend in the function with current and second approach and see which one is better.

From 2020-05-07-QuakeNet-%230ad-dev.log:

09:17 < Freagarach> StatusEffects was requested changes. The tooltip mayhem ;(
09:18 < Freagarach> I am unaware of the proper way. We don't want it hardcoded in the templates. The current implementation is very very ugly, code-wise.

If the problem is that there are too many combinations of strings, get the number of combinations down. Limit the problem to limit the solution.

You could remove the branching of %(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s %(stackability)s by using a comma-separated list, didn't that already happen in other places?
It will reduce the ability of translators to do something else than a comma separated list, and not allow english strings to use something other than commas as well if its not in the templates, but at least it would avoid the problem of unmaintainable string complexity.

Is the claim about 4 options in the templates actually true? Only those templates that have these StatusEffects component templates and for which aura/techs exist with that thing need those strings.
I.e. if there arent techs modifying all 4 values, then the strings for that also arent needed.
Those strings could be specified in a parent template to avoid duplication.

But the StatusEffect authors might also want to consider mods.
For a modder it would be more maintainable to not have to modify strings based on aura / tech authoring, a generic string would be more useful.
So eh, comma separated list will be more scalable I suppose.
Version 11410 of the diff (D2296?id=11410) was closer to that and perhaps better to use that than resigning the issue when there are only 3 options and all of them cant meet all requirements and desirements.
Comma separated values could prove to be the least problematic if one sees that there will be 20, 40 or similar amounts of strings needed in templates or JS branched objects/if statements only for this one line of tooltip.

Freagarach updated this revision to Diff 11804.Thu, May 7, 6:56 PM

Back to comma separated values in the tooltip. Note that the stackability can be part of the statusInfo but it is separate now because we use parentheses.

Version 11410 of the diff (D2296?id=11410) was closer to that and perhaps better to use that than resigning the issue when there are only 3 options and all of them cant meet all requirements and desirements.

Perhaps I am too easily deterred ^^'

Owners added a subscriber: Restricted Owners Package.Thu, May 7, 6:56 PM
Vulcan added a comment.Thu, May 7, 6:57 PM

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

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

Vulcan added a comment.Thu, May 7, 6:59 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/667/display/redirect

wraitii requested changes to this revision.Thu, May 21, 3:19 PM
wraitii added a subscriber: wraitii.

I think we have to accept the limited flexibility of tooltips for now. It's not like we support switching to a vertical writing system either, so I don't think we really need to think tooooo hard about ordering optional elements.
Fixing that would simply require a stronger translation system which we don't have right now.

I have a bunch of inline issues, and since I committed rP23681 there might be some bits to rebase (unsure). I'm requesting changes over the translate/sprintf thing, but this looks quite good to me.

binaries/data/mods/public/gui/common/tooltips.js
389

From my current understanding of our translation system, this is equivalent to:

tooltipAttributes.push(translate(template.Tooltip))

and requires Tooltip to be a translatable string pulled from elsewhere (which it is since rP23681)

392

same

393

same

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

meh... I would suggest UUID-like -> Timestamp + random bits, but I'm not sure it's actually faster in the general case.

I'm not entirely convinced by status effects stacking, to be honest. Can you be "more" on fire? It also seems like it would be ugly GUI wise.

Perhaps we could have "levels", where being "low-poison" and getting poisoned again moves you to "medium-poison" or something. But that seems like work for status effects v4.

I would personally remove stacking effects for now, but I'm willing to be overruled by others (though @Angen seemed to agree that this looks meh earlier)

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

That modifier-only bit seems like it should be moved elsewhere

This revision now requires changes to proceed.Thu, May 21, 3:19 PM
Freagarach updated this revision to Diff 11966.EditedFri, May 22, 9:45 AM
Freagarach marked 4 inline comments as done.
  • Rebased.
  • Split StatusEffects stuff in Attacking.js.
  • Simplified translations in tooltips.js.
  • Simplified test.

And to the one committing, please remove the executable format from the StatusEffectsReceiver.js and its test.

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

Yes, you can be more on fire ;) I've seen a piece of wood burn faster when lit on two sides ;) How is the level system different than stacking?
IMHO Stacking is quite essential to Status Effects, although I do agree that GUI-wise it can be improved further (like a number overlaying the icon of the effect to indicate the stacking number).

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/780/display/redirect

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 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_StatusEffectsReceiver.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/tests/test_StatusEffectsReceiver.js
|  66|  66| 
|  67|  67| 
|  68|  68| // Test adding multiple effects.
|  69|    |-	reset();
|    |  69|+reset();
|  70|  70| 
|  71|  71| // Damage scheduled: 0, 1, 2, 10 seconds.
|  72|  72| cmpStatusReceiver.ApplyStatus({
Executing section cli...

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

wraitii added inline comments.Fri, May 22, 9:58 AM
binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
70

You could define custom transitions with levels over stacking, or other interesting effects such as:
"Low-poison": you get slowed down
"Medium-poison": you also get HP damage
"High-poison": you contaminate others (TODO ;) ).

That's impossible with stacking.

  • for ... in in test.

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

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

Stan added a comment.Mon, May 25, 11:14 PM

After testing the current state (v2), I was wondering if there shouldn't be a a specific tooltip for the receiver. It feels weird to me that generic effects such as burning display "Iberian cavalry used flaming javelins" instead of saying something like, this unit is burning :D If that's not too much work maybe it could be in this patch...

http://irclogs.wildfiregames.com/2020-05/2020-05-25-QuakeNet-%230ad-dev.log

See this video https://upload.jabberfr.org/IDnsA5VBmAuf_XmA/status-effect-test.mp4 for reference

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

do you really need !! ?

binaries/data/mods/public/simulation/components/tests/test_Attack.js
132

strings

Freagarach planned changes to this revision.Thu, May 28, 7:52 AM

Look into the separation of tooltips.

I do agree with Stan, I ran into this weirdness when doing the i18n but splitting the strings between attack and defender didn't occur to me :/
In general this looks quite good, I do think my "level" idea is better than stacking but that'll be for v4 :p

I'll wait on you to do the tooltip pass for a final review and ultimately committing this (will give time to myself and people to test it of course).