Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An animation system for NUI #2306

Merged
merged 35 commits into from
May 13, 2016
Merged

Conversation

msteiger
Copy link
Member

@msteiger msteiger commented May 1, 2016

This PR adds an animation system to NUI, based on #2273. This is what the final result looks like: https://youtu.be/Pk2jjDyDpnA

Most of the changes were fairly straight-forward. One larger refactoring was made on NUIMangerImpl to separate the creation process from pushing a screen properly. This was required to trigger initialization on button click and to avoid being garbage collected during the animation phase.

@msteiger msteiger force-pushed the ui_animation_system branch from f6c7d97 to 4df7a2a Compare May 1, 2016 16:02
@GooeyHub
Copy link
Member

GooeyHub commented May 1, 2016

Hooray Jenkins reported success with all tests good!

@Cervator
Copy link
Member

Cervator commented May 1, 2016

Tests out good! Couple items:

  • Maybe we should add a graphics option to suppress menu animations? They are nice but a little slower to use than no animations + to the right person the fast flying menu might be uncomfortable.
  • The ESC key for me in-game isn't showing the pauseMenu any.... yeah what you just said @msteiger :D

@Cervator
Copy link
Member

Cervator commented May 1, 2016

On a related note so we don't forget: this would fix #2271

Should we perhaps throw together a quick wiki page or tutorial on this and/or animation in general? Not necessarily before merging this, but we could put some rough details in a separate issue to be completed later.

@Cervator Cervator added the Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience label May 1, 2016
@GooeyHub
Copy link
Member

GooeyHub commented May 7, 2016

Uh oh, something went wrong with the build. Need to check on that

@msteiger
Copy link
Member Author

msteiger commented May 7, 2016

All done! The PR for gestalt must be merged before Jenkins can build this PR.

The gimmicks from the video are not present in the code.

It would be great if @Cervator could check in the mega-workspace for compilation problems, because the API for NUIManager and HUD elements has changed a little.

@Cervator
Copy link
Member

Cervator commented May 8, 2016

Sure I can run the usual round of testing. We'd need the Gestalt PR both merged and an updated lib published + adjusted in our build.gradle here tho, right?

Looks like other recent merges caused conflicts somewhere but probably minor.

@viveret
Copy link
Contributor

viveret commented May 8, 2016

There could be a few different options for menu transitions:

  • Swipe left / right
  • Fade in / out
  • No animation

Otherwise everything looks great! Glad I could help!

@msteiger
Copy link
Member Author

msteiger commented May 8, 2016

There's a checkbox in the video settings menu now (default: off).

For testing: yes, that's how it should work. We can wait until this is sorted out though and test it afterwards.

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@msteiger
Copy link
Member Author

Ok - this is ready for testing! You need to enable animated menus in the video settings screen to see a difference.

@Cervator Cervator merged commit ff340ab into MovingBlocks:develop May 13, 2016
@Cervator Cervator added this to the v1.0.1 milestone May 13, 2016
@Cervator Cervator added the API label May 13, 2016
@Cervator
Copy link
Member

Merged at long last!

@viveret thank you so much for getting this started and thank you @msteiger for adding some additional tweaking on top :-)

I fixed what was probably a small merge quirk over getting some translation keys duplicated.

Did also detect an API violation - oh noes! protected abstract void initialise(); from CoreHudWidget was removed / moved in favor of void initialise(); in ControlWidget which is an interface. Spot the trouble? Method now became public meaning old implementers were trying to constrain the level of access.

Four modules in the lineup were affected: EventualSkills, InGameHelp, Tasks, and WorldlyTooltip - fix was simply changing protected to public in one class each.

Considering how trivial that fix is and how easily I could personally apply it to every known affected module I wonder if it would make sense to have a "trivial clause" while we're still in Alpha and have a harder time detecting API violations. We should probably aim to create a module that attempts to test every single API method so we can attach that module build to the engine PR builder. That should do the trick, so long as we keep that module accurate somehow. I'll submit a separate issue.

I let that slide for now since we're still learning and improving our tooling, plus I haven't even technically announced v1.0.0 formally yet ... 😊

@skaldarnar skaldarnar added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. and removed Api labels May 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Topic: UI/UX Requests, Issues and Changes related to screens, artwork, sound and overall user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants