[Don't Merge, IP] UI Animation System (Required for Terasology/Breathing#4 and #1639) #2271 #2273
Conversation
|
Can one of the admins please verify this patch? |
|
Neat :-) @GooeyHub: ok to test @viveret: should there be so many inner classes/interfaces or should they be in their own files? I see one or two occasionally. I actually have little to no clue on the involved best practices, it just stuck out. Might also be good to add in an example, possible via a PR to a module able to use it :-) |
|
@Cervator They will be definitely moved to their own files as this one grows. It is easier to have all the basic code in one file for faster editing. The implementations of all the methods will warrant a good amount of files. |
|
Refer to this link for build results (access rights to CI server needed): |
| @@ -0,0 +1,151 @@ | |||
| /* | |||
| * Copyright 2014 MovingBlocks | |||
There was a problem hiding this comment.
Not sure why this would ever happen in a newly created file
There was a problem hiding this comment.
I copy/pasted the licensing code from another file. Should it be not included?
There was a problem hiding this comment.
Nope, just change the year to 2016. If you've created this file from IntelliJ, a license header with the current year would be autogenerated,
|
I'm also wondering how someone would hook the update method of the animation somewhere it would be called automatically. EDIT: Nevermind, found it! |
| private int myRepeatCount; | ||
|
|
||
| private enum AnimState { | ||
| STOPPED, RUNNING |
There was a problem hiding this comment.
You may also add a PAUSED state here to will resume the animation rather than restart it - although that's purely an optional feature request
|
@rzats I've made a lot of changes you should check out |
|
Refer to this link for build results (access rights to CI server needed): |
| return v * (end - start) + start; | ||
| } | ||
|
|
||
| public void setStart(float val) { |
There was a problem hiding this comment.
Should be v for consistency, I suppose - same with setEnd().
Alternatively, just set them all to value.
There was a problem hiding this comment.
Actually, it should probably be "value" in both cases, as abbreviations make code less readable even though this case is trivial.
|
@viveret: this seems to be almost perfect in terms of checkstyling now 👍 Don't have much to say about functionality yet. |
|
@rzats Awesome! I'll see if I can make transitions for menus or buttons. |
|
@emanuele3d Do you know how I can inject a class that implements |
|
@Cervator @emanuele3d @rzats have any idea how I can hook animations into the engine? |
|
@viveret simple example at https://github.com/Terasology/Spawning/blob/master/src/main/java/org/terasology/spawning/SpawnerSystem.java#L58 :-) Since these are UI animations I suspect they could live in a System registered for client side only. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Viveret, and then after using @RegisterSystem, use @share on the class so that other systems can have the animation system injected and used. Like this...https://github.com/MovingBlocks/Terasology/blob/develop/modules/Core/src/main/java/org/terasology/logic/inventory/InventoryClientSystem.java |
|
Refer to this link for build results (access rights to CI server needed): |
|
Nice example! Works well, fun to see :D Looks like you got some good technical feedback too. I just like seeing new stuff! |
|
Made a note on a medium-priority API issue - otherwise this looks great! |
|
@rzats I couldn't find your comments! D: |
…te and decelerate interpolators
|
Refer to this link for build results (access rights to CI server needed): |
|
I added the potential todos from the hidden comment to the initial post :-) The link may not go to the right place at first as it seemingly relies on first loading then expanding the obsolete line comment I see a new commit in any case so maybe they're already done :D I think another comment got hidden when another commit updated something: https://github.com/MovingBlocks/Terasology/pull/2273/files/a36cac9381b063b4265d4c675f6eea2e0e565245#r58928885 - was @msteiger suggesting to finish the PR with minimal functionality then add more later |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks - I will try to review this over the weekend. Another interesting candidate for animation could be Hermite interpolation (see TeraMath.fadeHermite): |
|
@msteiger have you had a chance to look over it yet? |
|
Yes, I'm looking into it currently. I'd like to edit a few things and send a PR to you when I'm done. I think that this might be easier than commenting and asking for changes. It might still take a few more days, though. |
|
Sounds perfect. |
|
Can you please resolve the merge conflicts? I will put my changes on top then. |
|
Sure, I'll work on that now! @msteiger done! |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks for the reminder. I'm working on it at the moment over at msteiger#27. Should be ready in a few days. |
|
@msteiger So was my initial code before the refactor good or worthwhile? |
|
Chances are it would auto-close as merged anyway when the other PR is completed, unless the commits had been rebased too much or something :-) Yay animation in any case! |
UI Animation System (Required for Terasology/Breathing#4 and #1639), full-fills discussion at #2271