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

WIP - Reorganize the input system (no merg, feedback plz) #364

Closed
wants to merge 14 commits into from
Closed

WIP - Reorganize the input system (no merg, feedback plz) #364

wants to merge 14 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 16, 2016

Wip: Looking for feedback, Basically I want to know if this has a chance to be merged, if it needs some big changes or if you prefer to keep the old setup and backport the fixes ?

ToDo: Add docs back where needed
ToDo: Check if there is no obsolete code left
ToDo: Remove remaining 'Nitya Note: and ToDo:"


Generalize the core input

Input to a game can be many things and has some specialized requirements depending on the game and/or the platform. As it stand now the input is a core part of engo and it is hard to modify or extend.

The changes in this pull request isolate the core of the input and then provide a safe and clean way to use it. By doing this there is a cleaner path to extend, swap out or setup a secondary input system when required.

@Sendoushi mentioned this desire in gitter and it is used by me to push and pop input behavior depending on the game state, when attempting to do this in the old system it became messy due to the use of global variables that link it all up.

Minimizes string lookups

When looking at the benchmarks of the old system there is a big diffidence between the most optimal way of doing things and the worst way of doing things. This is mostly due to the string based lookups that happen every frame.

The new system ditches the string lookups in the game loop and moves them to the setup and loading section of the game. Once running all lookups are based on id's.

  • This also adds the option to check and implement fall-backs for systems that depend on some inputs to be setup. The ability to do this in turn helps when adding saving/loading of user preferences (See: The input demo)

Note: The optimal benchmarks where possible because the axes and buttons (ab)-used global variables. The new system no longer has an optimal or sub-optimal path, performance wise that benchmark would now sit between the two extremes

Other changes worth mentioning

  • Cleans up the testing to make it easier to add edge case.
  • Mouse buttons now act like key codes and can be used in the axis and button managers.
  • Solves the issue with out of order state changes that cause some states to be dropped. (Resolves Input Demo - This is broken on web builds #342)
  • Fixes behavior on the axis where holding down both min and max would not return zero. (I assume zero is the valid outcome of this edge case)
  • Extend axis with the option to check the state of an axis in the same way you can check state for codes and buttons. Plus the option to check state for a specific axis direction (min/max). (See: The input demo)
  • Fixes a issue in buttons where just pressed/released would trigger multiple times when interacting with a secondary key while holding down a primary key.
  • It touches on the proposal made in Keys namespace #349 by moving the codes in to a separated package and using the 'Key' and 'Mouse' prefix. (Resolves Keys namespace #349)
  • The way key and mouse codes are setup adds a safe and easy way to add custom codes to the action manager when needed. (Controllers, touche, ...)

Edge cases mentioned above are added to the new testing

Things I played with but left out

  • Act, Axis, Button Events: Hard to generalize, a generalized event system slows things down and a specialized system is to focused on one use case.
  • Combo Axis/Button and Sequence managers: When trying to make these more general they slowed down dramatically. These managers all have there own unique sets of behaviors, witch set of behaviors you need depends on the game or application you are making

Leaving these out of the engine and adding some specialized demos on how to achieve these behaviors would be preferable to me.

)

var (
// Time is the active FPS counter
Time *Clock

// Input handles all input: mouse, keyboard and touch
Input *InputManager
Input *InputMgr
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these Mgr should be Manager. More semantic and as such easier to understand.

Copy link
Author

@ghost ghost Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, using consistent suffixes (like Mgr) is helpful for someone maintaining the code, using Manager is annoying for a maintainer (More typing and longer, more complex lines to read). Using the full word is only helpful for people reading the code for the first time, learning the meaning of a consistent set of suffixes happens fast.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I don't agree. I'm against things like variable names as o or s because then I have to search for signatures or whatever. Even when I'm the guy who did the code. I always prefer things a little more verbose but easier to understand what they are.

Anyway, just my humble opinion!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that variable names need to be verbose, but I prefer using a short suffixes to add extra info on a object name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this sort of shortening. I'd strongly prefer having InputManager.

@coveralls
Copy link

coveralls commented Jun 16, 2016

Coverage Status

Changes Unknown when pulling 644bb9a on nitya-dev:nitya-input into * on EngoEngine:master*.

@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Changes Unknown when pulling a1e400b on nitya-dev:nitya-input into * on EngoEngine:master*.

@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Changes Unknown when pulling a1e400b on nitya-dev:nitya-input into * on EngoEngine:master*.

const StateJustIdle = State(2)
const StateJustActive = State(3)

////////////////
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't idiomatic godoc commenting

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, from the go docs:

Comments that are not adjacent to a top-level declaration are omitted from godoc's output, with one notable exception.

So godoc allows the user to add top level comments that are ignored. If they are not ignored this is a bug or they need to adjust there documentation. Now I do understand you do not like this comment style, I remember that from the last pr and I was planning to remove it when it was ready to be merged.

@ghost ghost changed the title Reorganize the input system WIP - Reorganize the input system (no merg, feedback plz) Jun 17, 2016
@ghost
Copy link
Author

ghost commented Jun 17, 2016

I tried, I'm to old for this social media bullshit. I have 5.5 months left to finish the port of the two games to golang. If that is done I will setup a repo with MIT code you can backport. gl

@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Changes Unknown when pulling f76fb58 on nitya-dev:nitya-input into * on EngoEngine:master*.

@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Changes Unknown when pulling 25dfab6 on nitya-dev:nitya-input into * on EngoEngine:master*.

@ghost
Copy link
Author

ghost commented Jun 17, 2016

This was my last push, if you want the changes I suggest you clone my repo and adjust/patch/doc it your self. I now remember why I dislike the github culture and I will be removing my account again soon.

@Noofbiz
Copy link
Member

Noofbiz commented Mar 15, 2018

Outdated and there's a better input system suggestion in #349 and #433

@Noofbiz Noofbiz closed this Mar 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keys namespace Input Demo - This is broken on web builds
6 participants