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

Mouse scrolling stuff #810

Merged
merged 1 commit into from Mar 16, 2015

Conversation

Projects
None yet
8 participants
@binary1248
Member

binary1248 commented Feb 27, 2015

Implemented support for horizontal mouse wheel scrolling as well as high-precision scrolling on Windows and OS X. Should fix (#95).

If you're wondering why there isn't support for high-precision scrolling on Unix, it's because I spent half a day fighting with the non-existant XCB Xinput documentation only to realize that most X servers probably might not even provide high-precision values yet.

As usual, OS X stuff hasn't been tested.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 27, 2015

Member

Isn't it confusing to have delta, deltaX and deltaY within the same structure? (don't reply "documentation", you know that people never read it...)

What do you think of

int delta;
Vector2f highPrecisionDelta;

Or we could even think of more modifications. For example, we could create two new events HorizontalWheelMoved and VerticalWheelMoved with float delta, and mark MouseWheelMoved as deprecated.

Member

LaurentGomila commented Feb 27, 2015

Isn't it confusing to have delta, deltaX and deltaY within the same structure? (don't reply "documentation", you know that people never read it...)

What do you think of

int delta;
Vector2f highPrecisionDelta;

Or we could even think of more modifications. For example, we could create two new events HorizontalWheelMoved and VerticalWheelMoved with float delta, and mark MouseWheelMoved as deprecated.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 27, 2015

Member

I think splitting the event might make the most sense. On Windows and Unix they are generated separately anyway, I don't know if OS X actually generates events with deltas along multiple axes simultaneously but I don't think it will be that big of a drawback anyway. On the plus side, if we split the events, the size of the structs/union will be reduced which is always a good thing 😛.

Member

binary1248 commented Feb 27, 2015

I think splitting the event might make the most sense. On Windows and Unix they are generated separately anyway, I don't know if OS X actually generates events with deltas along multiple axes simultaneously but I don't think it will be that big of a drawback anyway. On the plus side, if we split the events, the size of the structs/union will be reduced which is always a good thing 😛.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 28, 2015

Member

What's the reason for introducing a second member rather than just changing the data type? Considering the integer variable is unreliable, I'd just accept potentially breaking existing code, since it would be broken anyway. Also leave the name untouched - classic scrolling is vertical only. Add a second event for horizontal instead.

Member

MarioLiebisch commented Feb 28, 2015

What's the reason for introducing a second member rather than just changing the data type? Considering the integer variable is unreliable, I'd just accept potentially breaking existing code, since it would be broken anyway. Also leave the name untouched - classic scrolling is vertical only. Add a second event for horizontal instead.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 28, 2015

Member

What's the reason for introducing a second member rather than just changing the data type? Considering the integer variable is unreliable, I'd just accept potentially breaking existing code

It was discussed in #95 (comment).

I think splitting the event might make the most sense.

Yes, it definitely looks cleaner, and code using the new events won't break in SFML 3 when the old event is removed.

Member

LaurentGomila commented Feb 28, 2015

What's the reason for introducing a second member rather than just changing the data type? Considering the integer variable is unreliable, I'd just accept potentially breaking existing code

It was discussed in #95 (comment).

I think splitting the event might make the most sense.

Yes, it definitely looks cleaner, and code using the new events won't break in SFML 3 when the old event is removed.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 28, 2015

Member

Updated.

Still needs testing on OS X.

Member

binary1248 commented Feb 28, 2015

Updated.

Still needs testing on OS X.

Show outdated Hide outdated include/SFML/Window/Event.hpp Outdated
@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Feb 28, 2015

Member

At first sight the changes look good but I'll test it later (probably next week, though).

Member

mantognini commented Feb 28, 2015

At first sight the changes look good but I'll test it later (probably next week, though).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 28, 2015

Member

MouseHorizontalWheel or HorizontalMouseWheel? 😛

Member

LaurentGomila commented Feb 28, 2015

MouseHorizontalWheel or HorizontalMouseWheel? 😛

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 28, 2015

Member

I prefer having "Mouse" at the start 😛.

Member

binary1248 commented Feb 28, 2015

I prefer having "Mouse" at the start 😛.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 28, 2015

Member

Ok, I was just wondering if it didn't sound too weird.

Member

LaurentGomila commented Feb 28, 2015

Ok, I was just wondering if it didn't sound too weird.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Feb 28, 2015

Member

MouseWheelVertical+ MouseWheelHorizontal IMO - since both are MouseWheel so I wouldn't split that.

Member

MarioLiebisch commented Feb 28, 2015

MouseWheelVertical+ MouseWheelHorizontal IMO - since both are MouseWheel so I wouldn't split that.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 1, 2015

Member

Updated.

Member

binary1248 commented Mar 1, 2015

Updated.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 4, 2015

Member

Seems to work relatively well on OS X too. Just one thing: on Unix/Windows, do you get "null" events (where the delta is 0)? On OS X that's the case so you get spammed with those negligible events. I'm wondering if SFML should filter them or not... What do you think?

Member

mantognini commented Mar 4, 2015

Seems to work relatively well on OS X too. Just one thing: on Unix/Windows, do you get "null" events (where the delta is 0)? On OS X that's the case so you get spammed with those negligible events. I'm wondering if SFML should filter them or not... What do you think?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 4, 2015

Member

You will probably get them with the other OSs as well. If you have a high precision mouse in Linux, you will still get either 0.0 or 1.0 ticks. Don't know how it is on Windows exactly, but I can imagine values of 0 as well if the integer value can't represent the tiny movement you made on your wheel.

This way, we leave it up to the user to interpret what these values could mean. We can't know if it they might be meaningful to them or not, so better just give them everything and let them decide.

Member

binary1248 commented Mar 4, 2015

You will probably get them with the other OSs as well. If you have a high precision mouse in Linux, you will still get either 0.0 or 1.0 ticks. Don't know how it is on Windows exactly, but I can imagine values of 0 as well if the integer value can't represent the tiny movement you made on your wheel.

This way, we leave it up to the user to interpret what these values could mean. We can't know if it they might be meaningful to them or not, so better just give them everything and let them decide.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 4, 2015

Member

Good point.

Member

mantognini commented Mar 4, 2015

Good point.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Mar 4, 2015

Member

From a quick test I can't get a delta smaller than -1 or 1 with my Logitech mouse's wheel and the wheel steps disabled (Windows 8.1 x64). I only end up with -1 or 1 no matter how fast I roll the wheel or whatever driver setting I use (125 Hz polling vs. 1 kHz polling). Not sure whether that's normalized in the drivers or so though.

Member

MarioLiebisch commented Mar 4, 2015

From a quick test I can't get a delta smaller than -1 or 1 with my Logitech mouse's wheel and the wheel steps disabled (Windows 8.1 x64). I only end up with -1 or 1 no matter how fast I roll the wheel or whatever driver setting I use (125 Hz polling vs. 1 kHz polling). Not sure whether that's normalized in the drivers or so though.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 4, 2015

Member

It's completely up to the operating system what values it returns. MSDN documentation simply states that the OS can return values that are smaller than 120, and the developer should handle this properly. If the driver/OS doesn't want to provide smaller values through the message API, then there is nothing we can really do.

Member

binary1248 commented Mar 4, 2015

It's completely up to the operating system what values it returns. MSDN documentation simply states that the OS can return values that are smaller than 120, and the developer should handle this properly. If the driver/OS doesn't want to provide smaller values through the message API, then there is nothing we can really do.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 9, 2015

Member

@binary1248 Is there anything else to do here? Since it's only +134/−44 changes, I feel we could include it in 2.3. What do you think?

Member

mantognini commented Mar 9, 2015

@binary1248 Is there anything else to do here? Since it's only +134/−44 changes, I feel we could include it in 2.3. What do you think?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 9, 2015

Member

I have nothing more to add to this. If everyone else agrees this can be included in 2.3, then be my guest 😉.

Member

binary1248 commented Mar 9, 2015

I have nothing more to add to this. If everyone else agrees this can be included in 2.3, then be my guest 😉.

@mantognini mantognini added this to the 2.3 milestone Mar 9, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 12, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Mar 12, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 14, 2015

Member

The values seem indeed a bit odd. I'm getting either 1, 2, 3 or 4 for scrolling up and 545.113, 544.113, 543.113 or 542.113 for scrolling down.
For horizontal scrolling I get -545.113 for scrolling left and -1 for scrolling right.

So it doesn't do what the documentation comments say.

Member

eXpl0it3r commented Mar 14, 2015

The values seem indeed a bit odd. I'm getting either 1, 2, 3 or 4 for scrolling up and 545.113, 544.113, 543.113 or 542.113 for scrolling down.
For horizontal scrolling I get -545.113 for scrolling left and -1 for scrolling right.

So it doesn't do what the documentation comments say.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 14, 2015

Member

Turns out a cast was missing. 😁

Should work as expected now...

Member

binary1248 commented Mar 14, 2015

Turns out a cast was missing. 😁

Should work as expected now...

@zsbzsb zsbzsb referenced this pull request Mar 14, 2015

Closed

Track releasing 2.3 #70

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 14, 2015

Member

Seems to be working now. 👍

Member

eXpl0it3r commented Mar 14, 2015

Seems to be working now. 👍

@eXpl0it3r eXpl0it3r merged commit e17cc52 into master Mar 16, 2015

@eXpl0it3r eXpl0it3r deleted the feature/mouse_horiz_highpres branch Mar 16, 2015

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 20, 2015

Member

Why not one event type for both vertical and horizontal scrolling and an additional enum member denoting which one it was?

class Mouse
{
public:
   enum Wheel // <- new
   {
      HorizontalWheel,
      VerticalWheel,
   };
};

struct Event
{
   struct PreciseMouseWheelEvent // <- new
   { 
      float delta;
      int x;
      int y;
      Mouse::Wheel wheel;
   };

   struct MouseWheelEvent // <- deprecated
   {
      int delta;
      int x;
      int y;
   };
};

Having two identical structs is code duplication which eventually leads to combinatorial explosion. If you ever encounter a strange mouse model with a third wheel, you have to create yet another struct (and even with only two, it feels wrong to me).

Even more important, this approach is consistent to existing event APIs like MouseButtonEvent and JoystickButtonEvent which have members for the current mouse/joystick state, and one to know which button was pressed. Scrolling different mouse wheels are not different event types, the same way that pressing different buttons are not.

Member

Bromeon commented Mar 20, 2015

Why not one event type for both vertical and horizontal scrolling and an additional enum member denoting which one it was?

class Mouse
{
public:
   enum Wheel // <- new
   {
      HorizontalWheel,
      VerticalWheel,
   };
};

struct Event
{
   struct PreciseMouseWheelEvent // <- new
   { 
      float delta;
      int x;
      int y;
      Mouse::Wheel wheel;
   };

   struct MouseWheelEvent // <- deprecated
   {
      int delta;
      int x;
      int y;
   };
};

Having two identical structs is code duplication which eventually leads to combinatorial explosion. If you ever encounter a strange mouse model with a third wheel, you have to create yet another struct (and even with only two, it feels wrong to me).

Even more important, this approach is consistent to existing event APIs like MouseButtonEvent and JoystickButtonEvent which have members for the current mouse/joystick state, and one to know which button was pressed. Scrolling different mouse wheels are not different event types, the same way that pressing different buttons are not.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Mar 20, 2015

Member

I like @Bromeon's suggestion. :)

Member

TankOs commented Mar 20, 2015

I like @Bromeon's suggestion. :)

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 20, 2015

Member

I know this was merged already, but I have to say @Bromeon made a very good suggestion. I wouldn't be against changing it even though it was already merged into master. Yes - we would be breaking the API, but I think it would be fine as this API is hardly even established yet (I doubt anyone is using it yet).

Member

zsbzsb commented Mar 20, 2015

I know this was merged already, but I have to say @Bromeon made a very good suggestion. I wouldn't be against changing it even though it was already merged into master. Yes - we would be breaking the API, but I think it would be fine as this API is hardly even established yet (I doubt anyone is using it yet).

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 20, 2015

Member

I guess it makes sense, indeed. And since the API was not yet written in stone (only official release matter I'd say), we can change that easily.

Member

mantognini commented Mar 20, 2015

I guess it makes sense, indeed. And since the API was not yet written in stone (only official release matter I'd say), we can change that easily.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 21, 2015

Member

I could write a corresponding pull request. What do you think about the identifiers, are they okay? I extended the above snipped accordingly.

  • enum sf::Mouse::Wheel -- consistent to existing sf::Mouse::Button
  • struct sf::Event::PreciseMouseWheelEvent -- in addition to deprecated sf::Event::MouseWheelEvent
Member

Bromeon commented Mar 21, 2015

I could write a corresponding pull request. What do you think about the identifiers, are they okay? I extended the above snipped accordingly.

  • enum sf::Mouse::Wheel -- consistent to existing sf::Mouse::Button
  • struct sf::Event::PreciseMouseWheelEvent -- in addition to deprecated sf::Event::MouseWheelEvent
@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 21, 2015

Member

Those identifiers are fine in my book.

Member

zsbzsb commented Mar 21, 2015

Those identifiers are fine in my book.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 21, 2015

Member

👌

Member

mantognini commented Mar 21, 2015

👌

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 22, 2015

Member

I started a new branch feature/combined_mousewheel.

[Edit] See #837 for further discussion.

Member

Bromeon commented Mar 22, 2015

I started a new branch feature/combined_mousewheel.

[Edit] See #837 for further discussion.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 22, 2015

Member

I think for now the MouseWheelPreciseMoved event is ok but you have a good point: with v3 we should have

   struct MouseWheelEvent
   { 
      float delta;
      int x;
      int y;
      Mouse::Wheel wheel;
   };

(Devs would have in any case to adapt their code when migrating to v3 so something that small should not be an issue.)

Member

mantognini commented Mar 22, 2015

I think for now the MouseWheelPreciseMoved event is ok but you have a good point: with v3 we should have

   struct MouseWheelEvent
   { 
      float delta;
      int x;
      int y;
      Mouse::Wheel wheel;
   };

(Devs would have in any case to adapt their code when migrating to v3 so something that small should not be an issue.)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 23, 2015

Member

Might want to create your own PR and discuss further things there. 😉

Member

eXpl0it3r commented Mar 23, 2015

Might want to create your own PR and discuss further things there. 😉

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 23, 2015

Member

You're right. I didn't want to create a PR as long as nobody supported the idea, but it seems like people so far agree with it :)

Please continue the discussion in #837.

Member

Bromeon commented Mar 23, 2015

You're right. I didn't want to create a PR as long as nobody supported the idea, but it seems like people so far agree with it :)

Please continue the discussion in #837.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment