Page MenuHomeWildfire Games

GUI addon to animate objects proprieties.
Needs ReviewPublic

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


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)

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

Used in D1650: GridBrowser GUI addon

Test Plan

No test plan.

Diff Detail

Lint Skipped
Unit Tests Skipped

Event Timeline

nani created this revision.Dec 23 2018, 11:43 PM
wraitii added a reviewer: Restricted Owners Package.Dec 31 2018, 9:49 AM
bb added a subscriber: bb.Jan 5 2019, 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

1 ↗(On Diff #7060)

wouldn't complain about some JsDOC's

6 ↗(On Diff #7060)


9 ↗(On Diff #7060)


11 ↗(On Diff #7060)

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

15 ↗(On Diff #7060)

no need for braces

17 ↗(On Diff #7060)
19 ↗(On Diff #7060)

nuke spaces between [] more of them below

20 ↗(On Diff #7060)

inline the line above

23 ↗(On Diff #7060)

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

29 ↗(On Diff #7060)

if (setting.size) ?

33 ↗(On Diff #7060)

what is dict?

36 ↗(On Diff #7060)

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

no need for braces

54 ↗(On Diff #7060)

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

dt is misleading, rather use x

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

68 ↗(On Diff #7060)

also here dt is confusing => x

70 ↗(On Diff #7060)


77 ↗(On Diff #7060)

if (settings.color)

79 ↗(On Diff #7060)

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

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

109 ↗(On Diff #7060)

no space between function and (, same below

111 ↗(On Diff #7060)

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

112 ↗(On Diff #7060)

asking for a ternary

115 ↗(On Diff #7060)

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

116 ↗(On Diff #7060)


127 ↗(On Diff #7060)

-!== undefined

131 ↗(On Diff #7060)

globals on top

132 ↗(On Diff #7060)

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

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

137 ↗(On Diff #7060)


bb requested changes to this revision.Jan 5 2019, 12:12 AM
This revision now requires changes to proceed.Jan 5 2019, 12:12 AM
elexis added a subscriber: elexis.Jan 5 2019, 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.

33 ↗(On Diff #7060)

.size should always be an CSize no?

nani added a comment.EditedJan 5 2019, 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.Jan 5 2019, 3:15 PM

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

Two more comments.

39 ↗(On Diff #7060)

types.length I guess.

145 ↗(On Diff #7060)

Add a final newline, else people will complain ;)

nani updated this revision to Diff 7459.EditedFeb 7 2019, 4:13 AM

Now can modify text color, object sprite color type and size.

Added typical animation functionality as delay, duration, hooks, chained animations and such.

Planning loops in the future still pending how.

Btw, thank you @bb for the review. :)

nani updated this revision to Diff 7491.Feb 17 2019, 2:31 AM

Fixed some object references annoyance. Made sure shared animation settings don't get modified.

nani edited the summary of this revision. (Show Details)Feb 17 2019, 2:31 AM
nani edited the summary of this revision. (Show Details)
nani edited the summary of this revision. (Show Details)
Imarok added a subscriber: Imarok.Sun, Mar 3, 11:41 AM

It seems like you made the diff dependencies wrong around.
I think this revision should be the parent of GridBrowser GUI addon.
(Everyone gets confused by that at least once ;))

nani updated this revision to Diff 7548.Sat, Mar 16, 10:09 PM

Improved readability and code clarity.