Page MenuHomeWildfire Games

GUI addon to animate objects proprieties
Changes PlannedPublic

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

Details

Reviewers
bb
wraitii
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 D1650: GridBrowser GUI addon

Test Plan

No test plan.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7135
Build 11643: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan added a subscriber: Stan.Jan 5 2019, 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 ↗(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.Mar 3 2019, 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.Mar 16 2019, 10:09 PM

Improved readability and code clarity.

bb requested changes to this revision.Apr 5 2019, 12:56 PM

As elexis mentioned the color animation would in the vanilla game be dead code, thus that shouldn't be there. Ofcourse it could be useful later, so I would propose to implement the system for the size only, but keep it extendable so the color could easily be added when required.

Furthermore it would help a lot when there was a simple test case, which uses this object already, something like the gamesetup options panel or so

binaries/data/mods/mod/gui/common/animateGUI.js
38

type="string"

47

The spaces do not really help the readability and they are wrong from a style convention perspective, please nuke. One could consider newlines to group related settings together

49–52

Are those clones really required? Also instead of a default object somewhere hidden, I rather have them inline here i.e
settings.queue || false and similar for the others

75–77

Dislike the duplication in these switches, I guess one could define an object like

{
"size": { "setFunction": () => setFoo (), "getFunction": () => getFoo() }
etc.
}

to nuke some of that

90

Why not on init?

112

Is there any example of a guipage where we only have the size/color etc. in a string? if so we better add a fromString function to JSInterface_GUITypes, than reinvent the wheel here. IMO just assume the this.settings[property] is an object (and thus remove the now irrelevant code)

132

what is "x" here? Also what does this line do? (filter doesn't change the array, it creates a new one)

168

One could consider making the object a GUIColor and using the already existing toString on that

189

space goes after comma not before

205

instead of setting this parameter every tick, one could just set it onIntit, onStart and onComplete achieve the same result. (Together with the comments just below)

206

I guess with the below, this parameter becomes useless

207

Why not set a timer on init for this function? Same for onComplete? With this also the us of the this.done object should be reviewed

208–209

These two functions seem to have the same purpose: do something every tick (whether it is resizing an guiObject or calling a arbitrary function is indifferent for me), so merge the two

220–221

seemed to be a clean solution at first sight, however it is to limiting: consider the sliding background at the main menu, that is a perfect example of where such a gui component can be useful, however that needs an infinite liftime, so imo we have to allow that.

250

missing space

255

period

262

missing space

305

ticks can in principle have arbitrary length, so don't assume it will be quick enough: directly onComplete him

327–331

Don't predefine functions, let the calling page do that

383

missing space

This revision now requires changes to proceed.Apr 5 2019, 12:56 PM
nani marked 11 inline comments as done.Apr 5 2019, 4:04 PM
In D1702#74199, @bb wrote:

As elexis mentioned the color animation would in the vanilla game be dead code

Not committed but D1703 uses the three of them.

Furthermore it would help a lot when there was a simple test case, which uses this object already, something like the gamesetup options panel or so

Will try (or you could try the map browser diff D1703 which would show all the possibilities).

Thanks for reviewing.

binaries/data/mods/mod/gui/common/animateGUI.js
49–52

Yeah, clones are required, imagine you share an object properties for an animation setting, some of that might get modified so better be sure from the start you have two independent values.

I have moved the default to the top. IMHO having all defaults grouped allows to find/modify them faster.

90

There is no init, if you mean the constructor then is because the animation instance is created but might be sent to queue (doesn't start so its starting defaults can't be set)

112

Well the idea behind these two options is to make the end user life easier so he can choose what he thinks is more practical for him at that given moment.
Eg.
Case 1:
Just want to change the size of the right side:
animate("box", { "size": { "right" : 100}})

Case 2:
Want to change all properties:
animate("box", {"size": "0%0 0%0 0%100 0%100})

132

Filter returns the properties that you, in the properties related to the XML object (from the animation settings), have defined you want to change.

x is the normalized (unitary) time (with the AnimateGUIObject.curve function applied), x does a simple linear interpolation between the initial value and final value.

168

True, will try.

205

Seems worth the try.

220–221

More than infinite it could be said is a loop with no end, no? I was thinking the loop should be at the animation manager and not at the animation per se. The sliding background would be two animation chained that would loop from one to the other forever (would give more fine grained control too).

Opinions?

305

It will be processed at the next tick anyway no?

327–331

These are basic curve settings I don't think is bad to have them.

elexis added a comment.Apr 5 2019, 5:32 PM

I can't read the last two comments now, so I might repeat things mentioned by bb or say something that was responded to by nani. But I add some comments sooner rather than later:

Dead code can be useful to mods, but dead code that is useless code in any case should be nuked (if usage is too hypothetical, in particular if using it would require it to be redesigned or if the unused code is 25% where the other 75% have to be added and thus the 25% are likely wrong)

One file per prototype.

Encoding code is a bad pattern and it's nnot necessary - one can just specify a JS function x => f(x) instead of specifying some math as a string and parsing the string with logic that supports only few things while adding code to achieve that.

We should start with the use case analysis (do we need this to begin with), then the implementation design (if we need this, is it better in JS or C++).

Not sure if we need an animation queue rather than just starting immediately upon call.

If the code is written well, it doesn't have to add all the complexity to distinguish the property size/color/..., but it would be generic, and receive the property parsing as an argument. For example one function animate: (property, time) => newProperty.

binaries/data/mods/mod/gui/common/animateGUI.js
71

IIRC we use uppercase for prototype function names, one could take a look at the simulation.

79

s/switch/object (switch, at least in JS, is an anti-pattern as it fragments the scopes, I think I explained it in another diff a bit longer)

105

We don't use these alignments, because adding one longer line would require changing the whitespace of all other lines

140

.size

185

color.js has such transformations, all three functions if Im not mistaken

227

If adding comments, use JSdoc format /**

313

I suspect this function or parts from it were taken from some website. If that's the case, you should mention which one you used.
If ()\n
\nelse\n

316
324

need to distinguish space from other falsy values? === / !== only when == / != is insufficient (to have it consistent, mentinoed in other diffs)

331

Math.square, Math.pow(x, 3)? I heard that's supposedly slower though

353

This pattern should be avoided. There should be as few ambiguity as possible about the types when passing arguments.

local objects start with lowercase

400

no else after return

406

!nextAnimation

422

key?

426

do
{
...

476

Every statement on an own line.

global objects start with g_
no let in global context
Consitently using prototype everywhere seems good.

Naming needs to be more precise, "Animate" can be anything and nothing.

482

There is no such thing as a list in JS.
Don't add the JSdoc if it doesn't add information that isn't obvious from reading the code.
It adds 3 lines whereas the code is 2 lines, reader spent more than 100% additional time reading these lines without having gained something.

487

unnecessary double negation, also use || can be used instead of ?. also defaultSettings = {} in the argument. also default arguments are not so nice.

nani updated this revision to Diff 7679.Apr 5 2019, 8:10 PM
nani marked 4 inline comments as done.

Minor fixes from comments (will do bigger ones later) + gamesetup
example

Owners added a subscriber: Restricted Owners Package.Apr 5 2019, 8:10 PM
nani marked 18 inline comments as done.Apr 7 2019, 10:02 PM
nani added inline comments.
binaries/data/mods/mod/gui/common/animateGUI.js
71

uppercase or lower camel case ? Most of rmgen prototypes don't start with uppercase.

313

It was some stackoverflow post, can't seem to find it. Anyway, nucked for the moment,

324

Ok mom.

331

Quite slow.

422

yeah?

476

Is more of a function than an object, variable...

nani updated this revision to Diff 7699.Apr 7 2019, 10:20 PM
nani marked 4 inline comments as done.

Fixed all but the loop logic.

Harbormaster completed remote builds in B7135: Diff 7699.
nani retitled this revision from GUI addon to animate objects proprieties. to GUI addon to animate objects proprieties.Apr 8 2019, 2:46 AM
wraitii requested changes to this revision.Apr 22 2019, 1:20 PM

TBH this is too complex and too complicated for my tastes as it's written. I say 'remove stuff'.

  • You have only one AnimateGUIObject per GUI object, and try to handle adding new conflicting animations to that object, and things like that, and I don't see the point. Let all animations run in whatever order they've been added and do their thing. This drops a good 100 LOC and lets you eliminate a lot of complexity. If two animations are actually conflicting, that's a fault in the GUI code that added animations and needs to be handled there. Your job is running animations.
  • Likewise, I don't think we need queuing (if we want to, we can do things in onComplete). If there is some day an object with actually complex animations needs such that this is required, it'll probably be coded as new C++ component, you know. We're not making a 2D animation software but a GUI. Or we'll see then, but I doubt we'll see the day.
  • don't support multiple ways to pass setting(s). It's useless bloat.

I also don't quite understand why you call 'run()' on every tick? Is it because of bullet point #1 above?

I'm not _sure_ how generic we need to make "actions", haven't been quite able to understand how these work. We're working with JS, so I'd rather have straightforward code than params for this maybe.

This revision now requires changes to proceed.Apr 22 2019, 1:20 PM
nani planned changes to this revision.Wed, Oct 9, 2:11 PM