Page MenuHomeWildfire Games

StatusEffects v3 - More cleanup, allow stackable, upgrades.
ClosedPublic

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

Details

Reviewers
Angen
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23757: Handle stacking status effects, modifiers, some related fixes.
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.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.May 7 2020, 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.May 7 2020, 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.May 7 2020, 6:56 PM
Vulcan added a comment.May 7 2020, 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.May 7 2020, 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.May 21 2020, 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
380 ↗(On Diff #11804)

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)

385 ↗(On Diff #11804)

same

390 ↗(On Diff #11804)

same

binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
70 ↗(On Diff #11804)

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
147 ↗(On Diff #11804)

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

This revision now requires changes to proceed.May 21 2020, 3:19 PM
Freagarach updated this revision to Diff 11966.EditedMay 22 2020, 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 ↗(On Diff #11804)

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.May 22 2020, 9:58 AM
binaries/data/mods/public/simulation/components/StatusEffectsReceiver.js
70 ↗(On Diff #11804)

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.May 25 2020, 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 ↗(On Diff #11804)

do you really need !! ?

binaries/data/mods/public/simulation/components/tests/test_Attack.js
132 ↗(On Diff #11804)

strings

Freagarach planned changes to this revision.May 28 2020, 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).

Freagarach updated this revision to Diff 12174.Jun 6 2020, 3:44 PM

Different tooltip for affected entities.

I dislike the duplication, but I guess this is manageble?

Vulcan added a comment.Jun 6 2020, 3:45 PM

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

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

wraitii updated this revision to Diff 12175.Jun 6 2020, 4:12 PM

Straight rename to ApplierTooltip and ReceiverTooltip to match code.

Vulcan added a comment.Jun 6 2020, 4:13 PM

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

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

wraitii updated this revision to Diff 12176.Jun 6 2020, 4:14 PM

Forgot some lines in messages.json

wraitii accepted this revision.Jun 6 2020, 4:15 PM

This looks good to me. I think there is easily room for a V4 to clean up the icons more and such, but it makes status effects functional.
Thanks for the work. I'll leave this open for a day or so and then commit.

binaries/data/mods/public/simulation/templates/template_unit_infantry_ranged_archer.xml
49 ↗(On Diff #12174)
Vulcan added a comment.Jun 6 2020, 4:15 PM

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

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

wraitii updated this revision to Diff 12231.Jun 9 2020, 1:01 PM

Final upload wihtout the test template

Stan added inline comments.Jun 9 2020, 1:09 PM
binaries/data/mods/public/gui/common/tooltips.js
435 ↗(On Diff #12231)

Pending question about ===.

I also wonder if having enums and checking integers would have any perf impact. Probably irrelevant here.

binaries/data/mods/public/gui/session/selection_details.js
101 ↗(On Diff #12231)

Loop could probably be rewritten to use an index, but I guess it doesn't matter much.

binaries/data/mods/public/simulation/components/tests/test_Attack.js
132 ↗(On Diff #11804)

Still accurate, as it's nice to having warnings in test when you forget +values :)

binaries/data/mods/public/simulation/data/technologies/advanced_unit_bonus.json
27 ↗(On Diff #12231)

Remove this before committing.

Vulcan added a comment.Jun 9 2020, 1:11 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 406| 406| 			"statusInfo": tooltipAttributes.join(commaFont(translate(", "))),
| 407| 407| 			"stackability": getStatusEffectStackabilityTooltip(template)
| 408| 408| 		});
| 409|    |-	else
| 410|    |-		return sprintf(translate("%(statusName)s: %(statusInfo)s"), {
|    | 409|+	return sprintf(translate("%(statusName)s: %(statusInfo)s"), {
| 411| 410| 			"statusName": headerFont(translateWithContext("status effect", template.StatusName)),
| 412| 411| 			"statusInfo": tooltipAttributes.join(commaFont(translate(", ")))
| 413| 412| 		});
Executing section cli...

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

wraitii updated this revision to Diff 12232.Jun 9 2020, 1:23 PM

Stan's comments.

Vulcan added a comment.Jun 9 2020, 1:29 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-else-return):
|    | Unnecessary 'else' after 'return'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 406| 406| 			"statusInfo": tooltipAttributes.join(commaFont(translate(", "))),
| 407| 407| 			"stackability": getStatusEffectStackabilityTooltip(template)
| 408| 408| 		});
| 409|    |-	else
| 410|    |-		return sprintf(translate("%(statusName)s: %(statusInfo)s"), {
|    | 409|+	return sprintf(translate("%(statusName)s: %(statusInfo)s"), {
| 411| 410| 			"statusName": headerFont(translateWithContext("status effect", template.StatusName)),
| 412| 411| 			"statusInfo": tooltipAttributes.join(commaFont(translate(", ")))
| 413| 412| 		});
Executing section cli...

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

This revision was not accepted when it landed; it landed in state Needs Review.Jun 9 2020, 1:47 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the review and commit @wraitii :)