Page MenuHomeWildfire Games

GUI addon to animate objects proprieties.
Needs RevisionPublic

Authored by nani on Sun, Dec 23, 11:43 PM.

Details

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

For the moment can animate object size and color (as special case of sprite "color : r g b a").

Used in https://code.wildfiregames.com/D1650 to animate the grid.

Test Plan

No test plan.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

nani created this revision.Sun, Dec 23, 11:43 PM
wraitii added a reviewer: Restricted Owners Package.Mon, Dec 31, 9:49 AM
bb added a subscriber: bb.Sat, Jan 5, 12:11 AM

I guess every gui page should be able to access this file, also the mod-selection, thus mods/mod/gui/common should be the correct place

As there already are 4 places with sliding object, it seems indeed worth it to merge that code (rather similar to what happened to the tabButtons I suppose)

The idea of the patch seems good, however a ton of code style issues and optimisations

I wonder what would happen if someone adds two animations on the same object, if they do different things (one does color, other size), that is not problem otherwise, we could consider it a bug anyway, I guess

binaries/data/mods/public/gui/common/animateGUI.js
1

wouldn't complain about some JsDOC's

6

nuke

9

nuke

11

globals should be on top of the file, furthermore imo such a value should be something trivial like 0.

15

no need for braces

17
if
{
   foo();
}
else
{
   foo();
}
19

nuke spaces between [] more of them below

20

inline the line above

23

well same as above: linear seems to most trivial one to me...

29

if (setting.size) ?

33

what is dict?

36

You seem to like const, while they are technically correct (and preferred by some sources), we do not use them often, nor have we any convention on them, so meh

50

no need for braces

54

Wouldn't it be simple to loop over GUIObject.size and then get the value between the keyword and or % in settings.size (I am probably vague in what I mean, could give some example code if you want to)

Also I am not sure why you wish this to be a string, an object seems much easier to handle, and as this should always be related to an existing GUIObject which gives us a string object anyhow

If we really want it to be a string then we should write a separate function that transforms the string into an object

64

dt is misleading, rather use x

with (1-x) *start +x* end one could inline start and end (the comments should be nuked)

68

also here dt is confusing => x

70

GUIObjectSize

77

if (settings.color)

79

we already have the function you are looking for I guess: guiToRgbColor from color.js (that file might need to be relocated to the mod too)

87

oldColor.forEach((oldColor, index) => foo() ) ?

109

no space between function and (, same below

111

the denominator will always be this.settings.duration right?

112

asking for a ternary

115

no need to keep track of this: we could just check time >= this.time.end below too

116

-.0

127

-!== undefined

131

globals on top

132

ease-in-out doesn't say me anything, maybe we are better of by giving these function in the settings object, instead of a name referring to it (settings.curveParameterisation)

132–134

one line arrow function do not need braces and return keyword ("linear"; (x) => x)

137

global

bb requested changes to this revision.Sat, Jan 5, 12:12 AM
This revision now requires changes to proceed.Sat, Jan 5, 12:12 AM
elexis added a subscriber: elexis.Sat, Jan 5, 1:14 PM

We should avoid adding lots of unused code. Make it used if it's good. Lots of weird JS syntax, see bbs inline comments.
Why is color animation present?
Indirection can be removed, probably more readable to pass the settings arguments rather than one setting containing the arguments that are spread all over the place. In particular if you're going to use defaults.
Constructor function big, split into multiple functions.
Was wondering if it shouldn't be in C++, because all GUI object features are defined there. But why not, JS is versatile.
The use of eval makes me wonder if we can't remove that function in C++ so that unconscious or malicious people can't even try to use it.
AnimateGUI -> AnimateGUIObject, since the GUI is the entire thing.

No test plan.

There is at least one.

binaries/data/mods/public/gui/common/animateGUI.js
33

.size should always be an CSize no?

nani added a comment.EditedSat, Jan 5, 2:58 PM

Ok. Sorry for the seemingly confusing code. I'm still rewriting parts of the code and these weeks I'm half off, so will add a description about how is supposed to be used, what settings argument means and improve the code as bb and elexis suggested. Also give explanations of why it was done this way.

Stan added a subscriber: Stan.Sat, Jan 5, 3:15 PM

Agree with removing eval. uneval is useful for debugging though.

Two more comments.

binaries/data/mods/public/gui/common/animateGUI.js
39

types.length I guess.

145

Add a final newline, else people will complain ;)