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

Combined separate horizontal/vertical mouse wheel event types #837

Merged
merged 1 commit into from Apr 6, 2015

Conversation

Projects
None yet
6 participants
@Bromeon
Member

Bromeon commented Mar 23, 2015

Continues discussion in #810.

Suggests a change in the recently added scroll wheel API: instead of separate event types for horizontal/vertical scrolling, use one event type for both, with an additional member variable to differentiate them. I wrote some code to show the idea, however I have not thoroughly tested it (I don't even have a mouse with multiple scroll wheels). Overview:

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

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

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

Rationale

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.

API in the future

We should think about the future. Currently, there is a new event type called MouseWheelPreciseMoved, which sounds rather ugly. It is definitely a candidate for renaming to MouseWheelMoved in SFML 3, however that would break some code again. I don't know whether it might make more sense to extend the current MouseWheelMoved event with 2 new members:

   struct MouseWheelEvent
   { 
      int delta; // <- deprecated
      int x;
      int y;
      float highPrecisionDelta; // <- new
      Mouse::Wheel wheel; // <- new
   };

In SFML 3, delta would be removed. highPrecisionDelta is either left as-is, or renamed to delta (requiring code changes). That's why I considered using a different name that makes sense on its own, e.g. offset or similar. The drawback of this approach is that the relation to the delta is not immediately clear -- but this is just for the moment.

Minimal test snippet

#include <SFML/Graphics.hpp>
#include <iostream>

int main()
{
    sf::RenderWindow window(sf::VideoMode(640, 480), "SFML MouseWheel Test");
    window.setFramerateLimit(20);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::Escape || event.type == sf::Event::Closed)
            {
                return 0;
            }
            else if (event.type == sf::Event::MouseWheelMoved)
            {
                std::cout << "[MouseWheelMoved]         delta=" << event.mouseWheel.delta << std::endl;
            }
            else if (event.type == sf::Event::MouseWheelPreciseMoved)
            {
                std::cout << "[MouseWheelPreciseMoved]  delta=" << event.mouseWheelPrecise.delta
                    << "  wheel=" << (event.mouseWheelPrecise.wheel == sf::Mouse::VerticalWheel ? "VerticalWheel" : "HorizontalWheel") << std::endl;
            }
        }

        window.clear();     
        window.display();
    }
}

@Bromeon Bromeon added this to the 2.3 milestone Mar 23, 2015

@Bromeon Bromeon referenced this pull request Mar 23, 2015

Merged

Mouse scrolling stuff #810

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 23, 2015

Member

Regarding SFML 3, I think we don't need to keep the extra highPrecisionDelta: simply changing the type of delta to float would be enough. If someone don't care about precision (s)he can simply cast the delta value to int.

Member

mantognini commented Mar 23, 2015

Regarding SFML 3, I think we don't need to keep the extra highPrecisionDelta: simply changing the type of delta to float would be enough. If someone don't care about precision (s)he can simply cast the delta value to int.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Mar 23, 2015

Member

If someone don't care about precision (s)he can simply cast the delta value to int.

That's not true: it could end up with delta being always 0. To simulate the old behaviour you must accumulate the float delta, not just cast it.

Anyway, we don't really care, we can break stuff in SFML 3 ;)

Member

LaurentGomila commented Mar 23, 2015

If someone don't care about precision (s)he can simply cast the delta value to int.

That's not true: it could end up with delta being always 0. To simulate the old behaviour you must accumulate the float delta, not just cast it.

Anyway, we don't really care, we can break stuff in SFML 3 ;)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 23, 2015

Member

To simulate the old behaviour you must accumulate the float delta, not just cast it.

Good point. :-)

Member

mantognini commented Mar 23, 2015

To simulate the old behaviour you must accumulate the float delta, not just cast it.

Good point. :-)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 30, 2015

Member

@Bromeon could you rebase this branch?

Member

eXpl0it3r commented Mar 30, 2015

@Bromeon could you rebase this branch?

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 30, 2015

Member

So is has API decided on as being final? (trying to get ahead with CSFML)

Member

zsbzsb commented Mar 30, 2015

So is has API decided on as being final? (trying to get ahead with CSFML)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Mar 30, 2015

Member

I think it's safe to base your work on this branch. (@ others: correct me if I'm wrong)

Member

mantognini commented Mar 30, 2015

I think it's safe to base your work on this branch. (@ others: correct me if I'm wrong)

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 31, 2015

Member

👋

Member

eXpl0it3r commented Mar 31, 2015

👋

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 31, 2015

Member

Rebased. Still needs testing, especially from people with multiple mouse wheels :)

Member

Bromeon commented Mar 31, 2015

Rebased. Still needs testing, especially from people with multiple mouse wheels :)

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 31, 2015

Member

Are we sticking to MouseWheelPreciseEvent? I don't know... it just sounds a bit strange to me 😛. Maybe because I'm a native speaker. If there aren't any better alternatives, I'm fine with sticking with it.

Member

binary1248 commented Mar 31, 2015

Are we sticking to MouseWheelPreciseEvent? I don't know... it just sounds a bit strange to me 😛. Maybe because I'm a native speaker. If there aren't any better alternatives, I'm fine with sticking with it.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 31, 2015

Member

PreciseMouseWheelEvent sounds better to me.

Member

zsbzsb commented Mar 31, 2015

PreciseMouseWheelEvent sounds better to me.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 31, 2015

Member

I don't like it at all, either. I considered PreciseMouseWheelEvent, but I didn't want to break with the convention of <Device><InteractionMechanism>Event naming like JoystickConnectEvent, JoystickButtonEvent, MouseButtonEvent...

But maybe we can find a better word than Precise. It's actually an abbreviation of HighPrecision, which would definitely make identifiers too long... And Hp is probably not expressive enough ;)

Member

Bromeon commented Mar 31, 2015

I don't like it at all, either. I considered PreciseMouseWheelEvent, but I didn't want to break with the convention of <Device><InteractionMechanism>Event naming like JoystickConnectEvent, JoystickButtonEvent, MouseButtonEvent...

But maybe we can find a better word than Precise. It's actually an abbreviation of HighPrecision, which would definitely make identifiers too long... And Hp is probably not expressive enough ;)

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Mar 31, 2015

Member

I don't think that would be breaking it, after all 'precise' describes the device - not the event. It is a high precision mouse, so putting the two together in 'PreciseMouseXXX' sounds much more natural to me as a native speaker.

Member

zsbzsb commented Mar 31, 2015

I don't think that would be breaking it, after all 'precise' describes the device - not the event. It is a high precision mouse, so putting the two together in 'PreciseMouseXXX' sounds much more natural to me as a native speaker.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Mar 31, 2015

Member

That's true, but look at the current list of events:

        KeyPressed,             ///< A key was pressed (data in event.key)
        KeyReleased,            ///< A key was released (data in event.key)
        MouseWheelMoved,        ///< The mouse wheel was scrolled (data in event.mouseWheel) (deprecated)
        MouseWheelPreciseMoved, ///< The mouse wheel was scrolled (data in event.mouseWheelPrecise)
        MouseButtonPressed,     ///< A mouse button was pressed (data in event.mouseButton)
        MouseButtonReleased,    ///< A mouse button was released (data in event.mouseButton)
        MouseMoved,             ///< The mouse cursor moved (data in event.mouseMove)
        MouseEntered,           ///< The mouse cursor entered the area of the window (no data)
        MouseLeft,              ///< The mouse cursor left the area of the window (no data)
        JoystickButtonPressed,  ///< A joystick button was pressed (data in event.joystickButton)
        JoystickButtonReleased, ///< A joystick button was released (data in event.joystickButton)
        JoystickMoved,          ///< The joystick moved along an axis (data in event.joystickMove)
        JoystickConnected,      ///< A joystick was connected (data in event.joystickConnect)
        JoystickDisconnected,   ///< A joystick was disconnected (data in event.joystickConnect)
        TouchBegan,             ///< A touch event began (data in event.touch)
        TouchMoved,             ///< A touch moved (data in event.touch)
        TouchEnded,             ///< A touch event ended (data in event.touch)
        SensorChanged,          ///< A sensor value changed (data in event.sensor)

On one hand, the other mouse events do not start with PreciseMouse, even though they refer to the same high-precision mouse.

Of all device-related events, PreciseMouseWheelMoved would be the only one where the device is different from all others in the group. Apart from the inconsistency, what I'm a bit worried about is that it may be counter-productive, because users are used to get the related events by typing the device first. Those using auto-completion would never see PreciseMouseWheelMoved, but instead the deprecated MouseWheelMoved, which they may then use without even knowing about the other one. But I agree that it definitely sounds nicer.

Maybe more opinions on this?

Member

Bromeon commented Mar 31, 2015

That's true, but look at the current list of events:

        KeyPressed,             ///< A key was pressed (data in event.key)
        KeyReleased,            ///< A key was released (data in event.key)
        MouseWheelMoved,        ///< The mouse wheel was scrolled (data in event.mouseWheel) (deprecated)
        MouseWheelPreciseMoved, ///< The mouse wheel was scrolled (data in event.mouseWheelPrecise)
        MouseButtonPressed,     ///< A mouse button was pressed (data in event.mouseButton)
        MouseButtonReleased,    ///< A mouse button was released (data in event.mouseButton)
        MouseMoved,             ///< The mouse cursor moved (data in event.mouseMove)
        MouseEntered,           ///< The mouse cursor entered the area of the window (no data)
        MouseLeft,              ///< The mouse cursor left the area of the window (no data)
        JoystickButtonPressed,  ///< A joystick button was pressed (data in event.joystickButton)
        JoystickButtonReleased, ///< A joystick button was released (data in event.joystickButton)
        JoystickMoved,          ///< The joystick moved along an axis (data in event.joystickMove)
        JoystickConnected,      ///< A joystick was connected (data in event.joystickConnect)
        JoystickDisconnected,   ///< A joystick was disconnected (data in event.joystickConnect)
        TouchBegan,             ///< A touch event began (data in event.touch)
        TouchMoved,             ///< A touch moved (data in event.touch)
        TouchEnded,             ///< A touch event ended (data in event.touch)
        SensorChanged,          ///< A sensor value changed (data in event.sensor)

On one hand, the other mouse events do not start with PreciseMouse, even though they refer to the same high-precision mouse.

Of all device-related events, PreciseMouseWheelMoved would be the only one where the device is different from all others in the group. Apart from the inconsistency, what I'm a bit worried about is that it may be counter-productive, because users are used to get the related events by typing the device first. Those using auto-completion would never see PreciseMouseWheelMoved, but instead the deprecated MouseWheelMoved, which they may then use without even knowing about the other one. But I agree that it definitely sounds nicer.

Maybe more opinions on this?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Mar 31, 2015

Member

And before it slips through again: You might want to get rid of the tabs here. 😉

Edit: Nevermind, took care of it myself. 😛

Member

binary1248 commented Mar 31, 2015

And before it slips through again: You might want to get rid of the tabs here. 😉

Edit: Nevermind, took care of it myself. 😛

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Mar 31, 2015

Member

I'm fine with MouseWheelPreciseEvent, then again I'm not a native speaker either. I just think that terms like this don't need to be too close to English, but sure if there's a better alternative, let's hear it! 😉

Member

eXpl0it3r commented Mar 31, 2015

I'm fine with MouseWheelPreciseEvent, then again I'm not a native speaker either. I just think that terms like this don't need to be too close to English, but sure if there's a better alternative, let's hear it! 😉

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Apr 1, 2015

Member

Could we compromise with MousePreciseWheelEvent? Because it sounds entirely weird with 'precise' describing 'event'.

http://www.grammar.cl/english/adjectives-word-order.htm

because users are used to get the related events by typing the device first

Well that is where the 'RTFM' reply comes in 😉

Member

zsbzsb commented Apr 1, 2015

Could we compromise with MousePreciseWheelEvent? Because it sounds entirely weird with 'precise' describing 'event'.

http://www.grammar.cl/english/adjectives-word-order.htm

because users are used to get the related events by typing the device first

Well that is where the 'RTFM' reply comes in 😉

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Apr 1, 2015

Member

Another solution would be to use a synonym, so that it doesn't look strange and we can drop the deprecated one in SFML 3 without renaming the new one. For example, the new event could be MouseWheelScrolled. Of course this would add some confusion, but since no solution seems to be satisfying, I'm just giving more ideas.

Member

LaurentGomila commented Apr 1, 2015

Another solution would be to use a synonym, so that it doesn't look strange and we can drop the deprecated one in SFML 3 without renaming the new one. For example, the new event could be MouseWheelScrolled. Of course this would add some confusion, but since no solution seems to be satisfying, I'm just giving more ideas.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 1, 2015

Member

MouseWheelScrolled

"Scrolled" sounds better than "moved" anyway 👍

The question is how the event structure and its object would be named... MouseWheelScrollEvent and mouseWheelScroll? Or drop the "wheel" entirely?

I'm not sure whether we should not rename it in SFML 3... MouseWheelEvent still feels good, and the others are a bit longer than necessary...

Member

Bromeon commented Apr 1, 2015

MouseWheelScrolled

"Scrolled" sounds better than "moved" anyway 👍

The question is how the event structure and its object would be named... MouseWheelScrollEvent and mouseWheelScroll? Or drop the "wheel" entirely?

I'm not sure whether we should not rename it in SFML 3... MouseWheelEvent still feels good, and the others are a bit longer than necessary...

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 2, 2015

Member

I'm fine with either. Just make a decision soon enough. 😉

Member

eXpl0it3r commented Apr 2, 2015

I'm fine with either. Just make a decision soon enough. 😉

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Apr 2, 2015

Member

I can live with whatever, so just pick one and go and I will dutifully update the bindings with whatever you end up choosing.

Member

zsbzsb commented Apr 2, 2015

I can live with whatever, so just pick one and go and I will dutifully update the bindings with whatever you end up choosing.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 3, 2015

Member

Renamed:

(enumerator)    MouseWheelPreciseMoved  ->  MouseWheelScrolled
(event type)    MouseWheelPreciseEvent  ->  MouseWheelScrollEvent
(event object)  mouseWheelPrecise       ->  mouseWheelScroll

Suggested renaming in SFML 3:

(event type)    MouseWheelScrollEvent   ->  MouseWheelEvent
(event object)  mouseWheelScroll        ->  mouseWheel

Still needs testing.

Member

Bromeon commented Apr 3, 2015

Renamed:

(enumerator)    MouseWheelPreciseMoved  ->  MouseWheelScrolled
(event type)    MouseWheelPreciseEvent  ->  MouseWheelScrollEvent
(event object)  mouseWheelPrecise       ->  mouseWheelScroll

Suggested renaming in SFML 3:

(event type)    MouseWheelScrollEvent   ->  MouseWheelEvent
(event object)  mouseWheelScroll        ->  mouseWheel

Still needs testing.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 3, 2015

Member

Cool. Will test later.

Member

mantognini commented Apr 3, 2015

Cool. Will test later.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Apr 3, 2015

Member

👏 Those names look just fine me 👍

Member

zsbzsb commented Apr 3, 2015

👏 Those names look just fine me 👍

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Apr 4, 2015

Member

👍

Member

binary1248 commented Apr 4, 2015

👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 4, 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 Apr 4, 2015

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

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 4, 2015

Member

There is still no test feedback... Does somebody own a mouse with multiple wheels?

Member

Bromeon commented Apr 4, 2015

There is still no test feedback... Does somebody own a mouse with multiple wheels?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 4, 2015

Member

Except for a warning from clang, it works fine. 👍

SFML/include/SFML/Window/Mouse.hpp:69:24: warning: commas at the end of enumerator lists are a C++11 extension [-Wc++11-extensions]
Member

mantognini commented Apr 4, 2015

Except for a warning from clang, it works fine. 👍

SFML/include/SFML/Window/Mouse.hpp:69:24: warning: commas at the end of enumerator lists are a C++11 extension [-Wc++11-extensions]
@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Apr 4, 2015

Member

Thanks, fixed.

Member

Bromeon commented Apr 4, 2015

Thanks, fixed.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Apr 5, 2015

Member

Works fine!

#include <SFML/Graphics.hpp>
#include <iostream>

int main()
{
    sf::RenderWindow window(sf::VideoMode(640, 480), "SFML MouseWheel Test");
    window.setFramerateLimit(20);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if ((event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::Escape) || event.type == sf::Event::Closed)
            {
                return 0;
            }

            if (event.type == sf::Event::MouseWheelMoved)
            {
                std::cout << "[MouseWheelMoved]     delta=" << event.mouseWheel.delta << std::endl;
            }
            if (event.type == sf::Event::MouseWheelScrolled)
            {
                std::cout << "[MouseWheelScrolled]  delta=" << event.mouseWheelScroll.delta
                    << "  wheel=" << (event.mouseWheelScroll.wheel == sf::Mouse::VerticalWheel ? "VerticalWheel" : "HorizontalWheel") << std::endl;
            }
        }

        window.clear();
        window.display();
    }
}
Member

eXpl0it3r commented Apr 5, 2015

Works fine!

#include <SFML/Graphics.hpp>
#include <iostream>

int main()
{
    sf::RenderWindow window(sf::VideoMode(640, 480), "SFML MouseWheel Test");
    window.setFramerateLimit(20);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if ((event.type == sf::Event::KeyPressed && event.key.code == sf::Keyboard::Escape) || event.type == sf::Event::Closed)
            {
                return 0;
            }

            if (event.type == sf::Event::MouseWheelMoved)
            {
                std::cout << "[MouseWheelMoved]     delta=" << event.mouseWheel.delta << std::endl;
            }
            if (event.type == sf::Event::MouseWheelScrolled)
            {
                std::cout << "[MouseWheelScrolled]  delta=" << event.mouseWheelScroll.delta
                    << "  wheel=" << (event.mouseWheelScroll.wheel == sf::Mouse::VerticalWheel ? "VerticalWheel" : "HorizontalWheel") << std::endl;
            }
        }

        window.clear();
        window.display();
    }
}

@eXpl0it3r eXpl0it3r added the s:accepted label Apr 5, 2015

Combined separate horizontal/vertical mouse wheel event types
Instead of separate Event::MouseWheel{Vertical,Horizontal}Moved events, a single Event::MouseWheelScrolled event is used for all wheel-related events.
The new Mouse::Wheel enum is used to differentiate between mouse wheels.

@eXpl0it3r eXpl0it3r merged commit 22c9674 into master Apr 6, 2015

@eXpl0it3r eXpl0it3r deleted the feature/combined_mousewheel branch Apr 6, 2015

@Bromeon Bromeon removed their assignment Apr 30, 2015

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