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

Add manual update operations to keyboard/mouse state. #909

Open
SiegeLord opened this issue May 30, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@SiegeLord
Copy link
Member

commented May 30, 2018

This lets us reuse the terrible API for something more useful. Requires these functions (with obvious functionality:

al_init_mouse/keyboard_state(*state)
al_update_mouse/keyboard_state(*state, *event)
@tehsausage

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2018

I might be interested in doing this. Is this the kind of interface you were imagining?

ALLEGRO_KEYBOARD_STATE keyboard_state;
ALLEGRO_MOUSE_STATE mouse_state;
ALLEGRO_TOUCH_INPUT_STATE touch_state;

al_init_keyboard_state(&keyboard_state);
al_init_mouse_state(&mouse_state);
al_init_touch_input_state(&touch_state);

// ...

while (al_wait_for_event(event_queue, &e))
{
	al_update_mouse(&mouse_state, e);
	al_update_keyboard(&keyboard_state, e);
	al_update_touch_input_state(&touch_state, e);

	// ...

	if (al_key_down(&keyboard_state, ALLEGRO_KEY_ESC))
		exit();
}

I deliberately left out a set of functions to work with ALLEGRO_TOUCH_STATE - the structure which stores the state of an individual touch. It seems like it would be error-prone since you could pass it events for the wrong touch if you fail to filter by e.id (or e.primary). ALLEGRO_TOUCH_STATE could also be accidentally used in place of ALLEGRO_TOUCH_INPUT_STATE, which would lead to users storing the result of all touch events in a single state and end up with a confusing mess. It also mirrors the fact that there is no al_get_touch_state function to get the state of an individual touch (as opposed to al_get_touch_input_state which retrieves the state of all touches).

How do you feel about the fact the user isn't filtering events here before passing them to the update functions? Should the documentation / examples encourage the user to filter their events by source? e.g.

	if (e.source == al_get_mouse_event_source())
		al_update_mouse_state(e);
	else if (e.source == al_get_keyboard_event_source())
		al_update_keyboard_state(e);
	// etc...

Seems annoyingly redundant and like an extra source of errors. Ignoring un-related events seems like the more Allegro thing to do.

@SiegeLord

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2018

Yes, that's exactly what I was thinking, definitely no pre-filtering. Sounds good about leaving out ALLEGRO_TOUCH_STATE, don't want to introduce more non-deterministic state than we already have.

@tehsausage

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2018

A few more things:


al_init_*_state() seems like a bad addition: al_get_*_state() does the job of both initializing the state and setting it up with sane data. (Example: If you were to only use init/update, a game could start up thinking the mouse is at (0,0).)

Further issue: the display field of the keyboard/mouse state structs can't get initialized if al_get_*_state() is called before display creation, meaning al_update_*_state() is left in a bad situation where it cant match an event to your state's display. This also kills the possibility of adding al_init_*_state(), as the user would never be able to associate the blank state with a display.

Solution 1: I think this just requires clear documentation (to only use al_get_*_state() after display creation).

Solution 2: al_get_*_state() could be changed to zero-initialize if called without a driver+display, and al_update_*_state() would then not match events to their appropriate display. This leaves it up to the user to make sure not to mix states when using multiple displays.

Question: Which sounds better? I think I'd prefer Solution 1, however, Solution 2 could also make sense as the API already starts to break down with multiple displays.


• Joysticks have a state API with the same issue that ALLEGRO_TOUCH_STATE has: It needs to be filtered by the user per joy-stick. :(

Solution 1: I think this will just have to be a documented pit-fall since al_update_joystick_state() would be a pretty useful function.

Solution 2: I thought that something similar to ALLEGRO_TOUCH_INPUT_STATE could be added which combines the state of all joysticks in to a single structure with a fixed maximum number of joysticks. I considered that it wouldn't play nicely with the ability to re-configure joysticks, but the user already has to throw out their joystick state when re-configuring anyway.

Question: Should a joystick state update function be added at all? Should al_update_joystick_state() just be documented as easy to screw up? Or is the possibility for ALLEGRO_JOYSTICK_INPUT_STATE something that could be taken seriously.

Possibly al_update_joystick_state() could be added as unstable (or just left out), and then deprecated/removed at a later date if ALLEGRO_JOYSTICK_INPUT_STATE is found to be a reasonable addition?


• I guess I should add new functions as unstable API? Just making sure.

@SiegeLord

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

About al_init_*_state, I don't agree. Sure al_get_*_state can work, but it's always non-deterministic. The point of these new functions is to make these states predictable. Specifically, matching the state maintained by the user via al_update_*_state and the display's al_get_*_state is a non-goal (due to the latter being unpredictable anyway).

For mouse coordinates, perhaps they could be initialized to some invalid state (-1, -1?) or we could document that the state is semi-invalid until the first call al_update_*_state.

Not super concerned about the joystick API (as the API as is very bizzare right now), but what you're saying makes sense.

Yes, everything will be unstable, we'll have to play around with it either way.

@tehsausage

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Oops I was wrong about the relationships states have to multiple display, invalidating the first concern I had. A display doesn't need to created before getting state, and you don't need to manage one state per display.


matching the state maintained by the user via al_update_*_state and the display's al_get_*_state is a non-goal

Unless I mis-understand the point of this API, I think it should be a goal to make sure that the state the user is maintaining reflects reality. The only way to achieve that with al_init_*_state() would be synthesizing events on startup for, e.g., every joystick axis, the mouse location, and every held keyboard key, mouse button or joystick button.

Here's another scenario to consider: Imagine a Windows user has StickyKeys enabled and holding down a modifier key, and then they start up an Allegro game. The key is never going to get a down/up event, and the only way the game will find out about it is if it calls al_get_*_state.

(At least in terms of the API, that's the way you would get that information -- if a key that's being held down since startup isn't reflected in the keyboard state I think that's a bug that should be fixed)

or we could document that the state is semi-invalid until the first call al_update_*_state.

Each individual piece of state wouldn't be valid until the next event happens that updates that piece of state.

In the case of the mouse coordinates, games would have their custom-rendered mouse cursor start off in the top left corner of their game on startup which looks sloppy. People would end up having to get mouse coordinates on start-up to work around this, which defeats the point of making a replacement for a function that already does this.

(I checked and it doesn't seem like the Win32 mouse driver correctly sets the x/y values in the mouse state if the window opens with the mouse already over it, but if true that is again a bug that should be fixed IMO)

I'm not sure if I really understand the non-determinism problem if al_get_*_state() is only used once on startup. Sure, there's a small window where you could get redundant events (or dropped events if you call the functions in the "wrong" order), but that kind of inconsistency affects the entire event API due to event sources being registered non-atomically.

Of course, encouraging people to use the order which creates redundant events is important because removes the possibility of a "stuck key" in their state due to a dropped event. A redundant event has no concerns of breaking anything.

Thinking about solutions to allow al_init_*_state() to work properly -- they all seem much worse (e.g. trying to synthesize events) than just using al_get_*_state() in a location that allows only for redundant events:

al_register_event_source(&q, al_get_keyboard_event_source());
al_register_event_source(&q, al_get_mouse_event_source());
al_get_keyboard_state(&key_state);
al_get_mouse_state(&mouse_state);

I really cannot see why you would ever want "init" over "get"...

@SiegeLord

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

Yes, you make very good points. Let's omit the _init_ functions then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.