Comment viewing options

[Polling vs events]
In reality, the MouseDevice is event-driven, not polled. This is visible in the relevant platform-specific "drivers".

Apart from the MouseMove event, the interface itself supports both the event-based and the polled paradigms - you can either read the current state directly, or hook the relevant events and wait for notifications. The MouseMove event still needs some work, but I believe it can be made to work fine.

What do you feel is missing / do differently on this front?

[Deltas]
To tell you the truth, I hadn't considered what happens when passing a MouseDriver around. My gut instinct is that copying the MouseDriver is not a good design - it would be better to simply hook the relevant events (ok, MouseMove is missing so that may not be a complete solution).

The question is what should the deltas represent. There are two choices:
a. They show the relative movement since the last MouseMove event.
b. They show the relative movement since you last polled the delta.

The problem with (a) is that some platforms don't generate a MouseMove event when the mouse stops moving - which means the deltas stay stuck at non zero values! Needless to say, this is not very nice from a usability standpoint and that's why I changed the deltas to work like (b) in 0.9.1.

Now both of these approaches have merits. (a) makes sure that MouseMove events (once implemented) will always contain correct data. On the other hand, (b) ensures that you get correct data when polling the MouseDriver (since the underlying system is not poll-based).

[SetCurrentState]
What does that gain over individuals setters? Can't see a problem either way, I just think individual setters are more modular (i.e. why touch mouse position when the user just pressed a button?)

In any case, I'm interested in your changes to the MouseDriver. If I understand correctly, you'd prefer it to be more event-based? If so, what events would you add/modify?

At some level the mouse driver must be polled. If you really got every event, you would get event for each unit that the mouse moved. Since you get mouse changes representing several units of movement, there is clearly some aggregation of frequent small events into less frequent more significant events.

I think SetCurrentState needs to be atomic so that all state changes have registered before the events are fired.

Also, by using an atomic State Change, you can trigger all of your events in a logical succession.

I see the lack of atomicity as part of the reason why MouseMove isn't workable right now.

For example, lets say I get a mouse update from the driver. In that I see that the left button was pushed and the mouse was moved 100 units along the X axis. If MouseMove is enabled and I do granular updates, I would expect one of the two things to happen. Neither of which is necessarily accurate.
a) The button state is updated first. This triggers a MouseDown event, but the X and Y state values are stale.
b) The position state is updated first. This triggers a MouseMove event, but the Button state value is stale.

Using an Atomic state update, I would do the following.
a) record the current state.
b) update the mouse state with the new values.
c) compare current button states to previous button states and generate MouseUp events.
d) compare current button states to previous button states and generate MouseDown events.
e) compare current position states to previous position states and generate MouseMove events.

In the events, I would pass the readonly mouse state at the time of the event as opposed to the device itself. This would allow {X,Y,Wheel}Delta to be read multiple times without reseting.

Its really not a big deal to me. I can easily live with the current system and do my event logic in my code. I am trying to look for ways to contribute to the project and I thought this might be one way to help.

Actually, I appreciate the discussion and I think it's great that you wish to contribute your improvements back to the project. I've always thought that MouseDevice could use some work, but it's not easy to come up with a design that addresses all issues.

[SetCurrentState]
In reality, the scenario plays a little different: the OS doesn't report atomic mouse updates. That would only be possible with polling, but OpenTK opts to rely on OS events.

If you dig into the X11Input.cs or WMInput.cs files, you'll see that the OS registers different events/messages for mouse motion, button presses and releases. These events come in chronological order: the above scenario, where the user moves the mouse 100 units and presses a button would play out in one of four different ways:

The user moves the mouse 100 units along the X axis and then presses a button.

The user presses a button and then moves the mouse.

The user presses and holds a button, moves the mouse and releases the button at the end of the movement.

The user moves the mouse and, in the middle of the movement, presses a button.

In all four cases, the current implementation will update state in the correct order - exactly as an atomic SetCurrentState operation. (Note that mouse movement is reported in a distinct event from mouse buttons). The last one is the most tricky, but even that plays out correctly - the OS reports one or more movement events, then a button press/release, followed by one or more movement events. The first set of movement events ensure that the MouseDevice contains the correct position if you happen to read it during the button press event.

The only situation were an atomic function would behave differently than distinct setters occurs when the user presses several buttons at the same time: the individual setters would raise individual events, while the atomic operation could raise a single one. It happens, however, that both X11 and Win32 register mouse buttons in distinct events even when pressed at the same time - so this situation doesn't come up in practice.

Now that I read the above again, I'm not sure if I managed to explain this correctly - the gist is that the OS reports distinct events, so there's no difference between an atomic update function and distinct setters. Moreover, chronological order is observed so the data is never stale.

Can you think of any situations where the distinct setters would give incorrect results? I have a feeling I may have missed something obvious along the way and a second set of eyes always helps in these cases :D

[Events]
If I understand what you mean by "readonly mouse state", this is basically what the MouseDevice interface is (except for the *Deltas). The current events report e.g. which MouseButton was pressed/released and give a handle to an MouseDevice to read other state.

Say, if the deltas didn't reset, would there be a problem with passing MouseDevice around to access the mouse state?

[Deltas]
This is one place were we really need to find a nice solution as the current implementation is nothing but an ugly hack.

As you said, this hints at a fundamental design flaw in MouseDevice - the question is, is the flaw in the *delta implementation or the *delta semantics?

Let's imagine for a moment that the delta doesn't reset after it is read. What happens in the above example, where the user moves the mouse to the right by 100 units and presses a button?

As soon as the MouseMove event is raised, the XDelta becomes 100. When the MouseDown event is raised the XDelta is still 100, even though the mouse has stopped moving! If the user reads the XDelta during both the MouseMove event and the MouseDown event, he might assume that the mouse moved by 200 units, which is not correct. That was the case with OpenTK < 0.9.1 and it's clearly wrong.

For 0.9.1, I added the reset hack where the XDelta would become 0 after it was read. This takes care of the movement assumption (reading the XDelta multiple times will add up to 100 - the correct value), but (a) the getter modifies a public value (ouch!) and (b) there's an obvious race condition when copying the MouseDevice driver (which I missed - double ouch!).

The more that I think of it, the more that I feel that the deltas do not really make sense outside of the MouseMove events. If they only existed as parameters to the MouseMove event, both of these problems would disappear. More importantly, the MouseDevice would become a readonly view of the mouse state, as you recommend (and I absolutely agree).

[Proposed changes]

Mark the [X/Y/Wheel]Delta properties as obsolete.

Enable the MouseMove event.

Add the deltas as paremeters to the MouseMoveEventArgs.

Implement the MouseClick and MouseDoubleClick events.

Can you see any problems with these changes? Do you think these would cover what you have in mind for the API?

I was incorrectly assuming that you were using a system similar to DirectX where input devices are polled. Obviously, at the driver level they are polled, but if you are consistently seeing them already converted to event form across all OS's, it really doesn't matter if they were originally polled as the extra information is already lost.

I agree that without being able to peel back to the raw polled data, there would no benefit to my proposed atomic update. I also agree that your proposals numbered 1-3 would be the effective equivalent of what I was suggesting. I had not considered the case of MouseClick and MouseDoubleClick, but that seems like a reasonably simple addition.

As many games are simple rendering loops, it might be good to continue to allow access delta information without forcing event consumption.
I was thinking that one way to deal with the "reset" issue would be to allow the consumer obtain a state bookmark which could be passed back to the MouseDevice to obtain delta information.
If we internally stored the state in a readonly struct, it would be cheap to update and copy. This could serve as such a bookmark.

Off-topic: USB devices are polled but PS/2 are interrupt-driven. The latter do support polling, but unfortunately I don't remember if the x86 line offers an event driven mode (probably yes). I do remember that the, ugh, 8085 has such a mode (POS chip otherwise, compared to the RISC lines of the time).

Anyway, by changing mouse buttons into a bitmask, we can store the state bookmark in 28-36 bytes (3 or 4 axis x2 for deltas, plus the bitmask) - or even half that if we pack axis information in short fields. It's quite workable.

That said, it might be simpler to just let the user calculate deltas himself when not using events:

I've been toying with the proposed interface a bit and it looks good. I've also streamlined the events a bit so that they look closer to the WinForms Mouse API, which should make the MouseDevice a bit more accessible.

One issue that came up was what the "sender" argument should actually hold. Right now, it indicates the MouseDevice that raised the event, but I think it might be more useful to indicate the *Window* this event originated from. Suggestions?