Added methods to set the cursor type/image #827

binary1248 · 2015-03-15T05:52:10Z

Supersedes #784.

Thanks to @fundies for the initial draft implementation. I originally wanted to attribute the commit to him, but there were simply too many changes in the end.

Resource leaks should be gone now, and general code style is fixed up. Like #784, this is still missing the OS X implementation. Hopefully @mantognini or someone else can fill in the gaps here and here. It can't be nearly as horrible as the Windows or Unix implementations. 😛 This might help a bit.

This implements #269.

fundies · 2015-03-15T06:42:20Z

I don't care about credit, all I wanted was an implementation. Thanks.

LaurentGomila · 2015-03-15T08:38:24Z

What would be nice now is to get rid of Window::setMouseCursorVisible, and just add None to the Window::Cursor enum.

MarioLiebisch · 2015-03-15T09:09:14Z

@LaurentGomila I like that idea.

But before adding/changing something twice, how about creating a sf::Cursor class rather than an enum? Cursors aren't just images, there's also a position for the clicks.

~~It could be expanded later on to load custom textures as hardware cursors. But on the other hand this could be a 1:1 replacement of the existing enum as well. What do you guys think?~~

Edit: Just seen that there is a member for loading cursor images, but I'd rather see this wrapped into their own representation similar to things like sf::Color.

binary1248 · 2015-03-15T09:41:03Z

What would be nice now is to get rid of Window::setMouseCursorVisible, and just add None to the Window::Cursor enum.

We can't get rid of it just yet, since it would break the API. We can however make it have the same function as setting the cursor to None or something other than None just for the transition phase.

But before adding/changing something twice, how about creating a sf::Cursor class rather than an enum? Cursors aren't just images, there's also a position for the clicks.

Aren't you over-engineering a bit? For us, and the operating system, the "mouse" is what is used to represent an input device. It has a position that can be queried and set. Strictly speaking, we don't even have to see anything to be able to use it. The cursor is nothing other than a sprite that just tracks the position of the mouse. Combining both input and output into the same class would be coupling functionality where it isn't really necessary.

It could be expanded later on to load custom textures as hardware cursors.

So... how is that any different to what this PR does? This PR already loads custom image data into the operating system cursor which is as "hardware" as hardware gets. There isn't anything else that is more hardware than this.

But on the other hand this could be a 1:1 replacement of the existing enum as well.

What existing enum?

LaurentGomila · 2015-03-15T10:56:08Z

We can't get rid of it just yet, since it would break the API. We can however make it have the same function as setting the cursor to None or something other than None just for the transition phase.

Ah right. So either we do nothing and implement it in SFML 3, or we add None, mark setMouseCursorVisible as deprecated and forward it to setMouseCursor(None).

MarioLiebisch · 2015-03-15T12:08:20Z

Combining both input and output into the same class would be coupling functionality where it isn't really necessary.

You misunderstood me. It would be just another resource, representing a cursor by holding/using a sf::Image (similar to the relation of sf::Image and sf::Texture) or a system specific constant (i.e. built-in cursors).

So... how is that any different to what this PR does?

I wrote that line before noticing that this is already in the PR, I just didn't edit it out.

What existing enum?

The one introduced by this PR.

mantognini · 2015-03-15T15:03:07Z

@binary1248 I'll look into it hopefully in a week or two but no promise there. The implementation looks quite easy so if there's another Mac dev around, (s)he could probably do it too since the link in the description gives 75% of the implementation. 😉

binary1248 · 2015-03-15T15:39:55Z

What do the others think of providing a separate sf::Cursor class? It could be useful in situations where developers have to switch between cursor types very often and don't want the overhead of having to reload them every single time. Also, like @MarioLiebisch said, the system cursors can be provided as constant value sf::Cursors, although I still don't know how they would be constructed in that case without enums or some really ugly hacks.

@mantognini: 75% of the implementation for the setting the cursor to a system cursor. 😉 The bigger part is creating a cursor from a custom bitmap, and that part isn't really mentioned in there much.

LaurentGomila · 2015-03-15T15:49:57Z

What do the others think of providing a separate sf::Cursor class? It could be useful in situations where developers have to switch between cursor types very often and don't want the overhead of having to reload them every single time.

I'd say let's start simple. It's less convenient to have to load and handle the lifetime of an additional resource, than call a simple function. We can go back to this if feedback shows that it is needed.

mantognini · 2015-03-15T15:55:43Z

The bigger part is creating a cursor from a custom bitmap, and that part isn't really mentioned in there much.

Yes, but since setting an icon of a window already provides the code to load a bitmap and that we have -[NSCursor initWithImage:(NSImage *)newImage hotSpot:(NSPoint)aPoint], that should not be more than 25% of the total work. :-)

As for sf::Cursor class, I tend to agree with Laurent.

fundies · 2015-03-15T16:01:28Z

I started to attempt an os x implementation. Setting the cursor was fairly easy. However, I had trouble getting it to reset when the pointer was outside the window and gave up...

mantognini · 2015-03-15T16:22:45Z

@fundies This is (or at least should be) already handled by the gist (indirectly) linked in the description. ;-)

binary1248 · 2015-03-15T18:49:41Z

It's less convenient to have to load and handle the lifetime of an additional resource, than call a simple function.

That is if the user really only bothers having a single cursor they never intend to change in the first place. As soon as they start interchanging cursors, they will have to manage the lifetime of their image data anyway, and having cursor objects will save them a (potentially) expensive pixel copy each time they change.

We can go back to this if feedback shows that it is needed.

The problem is that if we wait for feedback after this is in the public API, we won't be able to get rid of it that easily. 😉 And if we really expect people to come onto the pull request tracker to tell us what they think, well... we might end up waiting a while for opinions. 😛

mantognini · 2015-03-17T14:36:06Z

I guess an extra level of abstraction couldn't hurt... it would also ease the creation of cursors from image/texture but more importantly be more consistent with the way we handle resources.

LaurentGomila · 2015-03-17T15:12:40Z

In this case, can the one-liner call still work?

void Window::setMouseCursor(const sf::Cursor& cursor);

// with implicit constructor:
window.setMouseCursor(sf::Cursor::Arrow);

// with explicit constructor:
window.setMouseCursor(sf::Cursor(sf::Cursor::Arrow));

If this is compatible with the way internal cursor resources are managed, this could be nice to have, for all these users that won't need to optimize this bit.

mantognini · 2015-03-17T15:30:32Z

I guess there would be two constructors in sf::Cursor, one implicit taking a enum and an explicit one taking an image or a bitmap. So both line would work. What do you think?

MarioLiebisch · 2015-03-17T15:32:32Z

Yep, sounds good. :)

binary1248 · 2015-03-17T15:34:26Z

I guess we could make that work. XCB cursor resources get copied over to the server, so they could be destroyed directly after the call and nothing would go wrong. Windows cursors on the other hand might require a bit of reference counting in the Impl since it isn't a good idea to destroy the image store before the cursor is put out of use (haven't tried, but documentation warns about this). Don't know about the lifetime requirements on OS X though, but if it's the same as Windows, we can just use the same reference counting on all platforms.

mantognini · 2015-03-17T18:28:43Z

Don't know about the lifetime requirements on OS X though, but if it's the same as Windows, we can just use the same reference counting on all platforms.

Generally speaking with OS X, the reference counter is already tangled in the API so there is not much extra work to do. Thus you don't need to worry about it when implementing the Windows part. 😉

eXpl0it3r · 2015-09-16T20:34:39Z

What's the final verdict here? Also the branch needs rebasing.

Bromeon · 2015-09-16T21:32:59Z

include/SFML/Window/Window.hpp

+        Arrow,                  ///< Arrow cursor (default)
+        ArrowWait,              ///< Busy arrow cursor
+        Wait,                   ///< Busy cursor
+        Text,                   ///< I-beam, cursor when hovering over a field allowing text entry


Is that not called "caret"? Or is caret only the blinking thing indicating where text is going to be inserted?

the blinking thing indicating where text is going to be inserted

This.

I would rename 'Text' to 'IBeam' directly in the enum. It makes more sense IMO. (not sure if it helps, but it is the way the .NET framework also calls it)

I personally think "Text" is fine as a name. It describes the cursor's purpose pretty well.

Bromeon · 2015-09-16T21:35:49Z

Regarding the signature

void setMouseCursor(const Uint8* pixels, unsigned int width, unsigned int height,
                    Uint16 hotspotX, Uint16 hotspotY);

Too bad this is done in the window module, otherwise we could have used

void setMouseCursor(const Image& image, Vector2u hotspot);

But even when sticking to the Uint8 pointer, it's weird to mix unsigned int with Uint16, and unnecessary to do everything on a low level. Why not

void setMouseCursor(const Uint8* pixels, Vector2u size, Vector2u hotspot);

LaurentGomila · 2015-09-17T05:56:30Z

Didn't we want to add Cursor::None and deprecate setMouseCursorVisible?

Bromeon · 2015-09-17T06:44:40Z

Didn't we want to add Cursor::None and deprecate setMouseCursorVisible?

Sounds good, the question is: what would setMouseCursorVisible(true) do? I'd say setMouseCursor(Arrow) if the cursor is not visible, and do nothing otherwise.

Also, there was a discussion about a dedicated sf::Cursor class. Even if we don't implement it, I would suggest to move the sf::Window::Cursor enum out of the sf::Window class into a sf::Cursor namespace, for the following reasons:

sf::Cursor::Text is much more expressive than sf::Window::Text.
We don't bloat the sf::Window and sf::RenderWindow scopes with attributes not directly related to a window.
If we decide to go with a dedicated cursor class later, we can simply transform the namespace sf::Cursor into a class, and code will continue to work.

mantognini · 2017-03-05T14:46:43Z

This PR was rebased onto master, the Windows implementation was rewritten based on @binary1248's previous work and the X11 implementation is new. Please do test and commend on both of those implementations as this is not my area of expertise!

You can use mantognini/SFML-Test-Events on the set_cursor branch as a basis to test those features. (The relevant bits are around line 500 of main.cpp.)

eXpl0it3r

This is a pure code-reading review. I'll test the implementation later on Windows.

eXpl0it3r · 2017-04-04T12:58:54Z

include/SFML/Window/ContextSettings.hpp

@@ -25,6 +25,7 @@
 #ifndef SFML_CONTEXTSETTINGS_HPP
 #define SFML_CONTEXTSETTINGS_HPP

+#include <SFML/Config.hpp>


Is that some left-over, or why is this here?

That might well be, yes! I'll mark this as a requested change and deal with it later.

Actually, now I remember: because I changed the order of some includes, this file no longer compiled (it's using Uint32). So this is actually needed and abides to our documented inclusion order.

eXpl0it3r · 2017-04-04T13:02:23Z

include/SFML/Window/Cursor.hpp

+    /// \return a reference to the OS-specific implementation
+    ///
+    ////////////////////////////////////////////////////////////
+    const priv::CursorImpl& getImpl() const;


Do we do this similarly in other classes?

You mean deferring to an Impl class? Yes, we did it for thread, input, sensor, window, ...

I meant, providing an explicit getImpl() function for a friended class.

Ah, that's just to dispatch in one place the actual CursorImpl for the WindowImpl. I think we didn't need this before because all platform dependent types were already "inside" WindowImpl, but this is not the case here.

eXpl0it3r · 2017-04-04T13:06:43Z

include/SFML/Window/Window.hpp

+    /// \see sf::Cursor::loadFromPixels
+    ///
+    ////////////////////////////////////////////////////////////
+    void setMouseCursor(const Cursor& cursor);


Did we discuss why we only have a setter and no getter?

Is this a real reference or are we copying the cursor internally? We might want to document whether the cursor object can be discarded or needs to live on.

Did we discuss why we only have a setter and no getter?

I don't think we did. But in any case, we don't do it for the other properties (e.g. visibility, ...).

We might want to document whether the cursor object can be discarded or needs to live on.

The doc reads:

The cursor must not be destroyed while in use

and

The behaviour is undefined if the cursor is destroyed while in use by the window.

;-)

eXpl0it3r · 2017-04-04T13:09:57Z

include/SFML/Window/Cursor.hpp

+    /// hotspot is the pixel coordinate within the cursor image
+    /// which will be located exactly where the mouse pointer
+    /// position is. Any mouse actions that are performed will
+    /// return the window/screen location of the hotspot.


I assume hotspot is a technical and commonly used term, correct?

Correct: it's used by Linux, Windows and Apple.

eXpl0it3r · 2017-04-04T13:14:10Z

src/SFML/Window/Cursor.cpp

+////////////////////////////////////////////////////////////
+bool Cursor::loadFromPixels(const Uint8* pixels, Vector2u size, Vector2u hotspot)
+{
+    if ((pixels == 0) || (size.x == 0) || (size.y == 0))


What's with the case where the hotspot isn't within the given size?
If it can be outside the given size, why can't there be negative values (Vector2u)?
If it can't be outside the given size, we probably want to either fail (don't hide error, but unclear failure) or limit to the max size (hiding error).

Some OS might support a hotspot outside the image, but it wouldn't make sense in practice. Here I think I followed what @binary1248 did in his initial version, but for V3 we might reconsider this and throw something instead when the values are not the one expected.

eXpl0it3r · 2017-04-04T13:16:09Z

src/SFML/Window/OSX/CursorImpl.mm

+{
+    switch (type)
+    {
+        default: return false;


Shouldn't the default be at the end of the case list? (Isn't it even legal to always use default if the cases are written after the default? Or maybe that's just some old C stuff, I've once read somewhere.)

It's perfectly valid C++98 code. I think we do elsewhere as well, but in any case I think it improves readability. What do you think?

eXpl0it3r · 2017-04-04T13:20:08Z

include/SFML/Window/Cursor.hpp

+    /// \warning On Unix, the pixels are mapped into a monochrome
+    ///          bitmap: pixels with an alpha channel to 0 are
+    ///          transparent, black if the RGB channel are close
+    ///          to zero, and white otherwise.


So Linux only supports monochrome cursors?

As far as I could find in the X11 documentation, yes.

On Windows it's hardware dependent, although I don't think we've still got any current and supported GPUs that don't support colored cursors.

I took @binary1248 impl for Win32 so I didn't look at the documentation that much, but it appears you're right:

Cursors can be either monochrome or color, and either static or animated. The type of cursor used on a particular computer system depends on the system's display. Old displays such as VGA do not support color or animated cursors. New displays, whose display drivers use the device-independent bitmap (DIB) engine, do support them.

Question is, should we document this or do we assume such old systems are not used with SFML?

I'd say it's safe to ignore, considering we want to use modern OpenGL anyway? Even my old 1 MB Cirrus Logic from 1996 supported colored/animated hardware cursors, although I remember some later games claiming otherwise. :)

eXpl0it3r · 2017-04-04T13:21:10Z

src/SFML/Window/Unix/CursorImpl.cpp

+    XFreePixmap(m_display, maskPixmap);
+
+    // We assume everything went fine...
+    return true;


So there's really no way this function could ever fail?

X11 being async, I think we simply cannot check for failure. Maybe some X11-guru can confirm that, but I've found nothing in the doc while writing this code to catch errors, sadly.

eXpl0it3r · 2017-04-04T13:21:47Z

src/SFML/Window/Unix/CursorImpl.cpp

+    unsigned int shape;
+    switch (type)
+    {
+        default: return false;


Again, should come after the cases.

eXpl0it3r · 2017-04-04T13:27:55Z

src/SFML/Window/Unix/WindowImplX11.hpp

@@ -265,7 +274,8 @@ class WindowImplX11 : public WindowImpl
    XIC        m_inputContext;   ///< Input context used to get unicode input in our window
    bool       m_isExternal;     ///< Tell whether the window has been created externally or by SFML
    int        m_oldVideoMode;   ///< Video mode in use before we switch to fullscreen
-    Cursor     m_hiddenCursor;   ///< As X11 doesn't provide cursor hidding, we must create a transparent one
+    ::Cursor   m_hiddenCursor;   ///< As X11 doesn't provide cursor hidding, we must create a transparent one
+    ::Cursor   m_lastCursor;     ///< Last cursor used -- this data is not owned by the window and is required to be always valid


Why exactly is it ::Cursor here?

To avoid a clash with sf::Cursor and X11 cursor, which lives in the global namespace.

While at those lines, I'm pretty sure it's "hiding" vs. "hidding". ;)

mantognini

Remove #include <SFML/Config.hpp> from ContextSettings.hpp.

this change is actually needed.

mantognini

Typo to fix

mantognini · 2017-04-05T07:46:48Z

src/SFML/Window/Unix/WindowImplX11.hpp

@@ -265,7 +274,8 @@ class WindowImplX11 : public WindowImpl
    XIC        m_inputContext;   ///< Input context used to get unicode input in our window
    bool       m_isExternal;     ///< Tell whether the window has been created externally or by SFML
    int        m_oldVideoMode;   ///< Video mode in use before we switch to fullscreen
-    Cursor     m_hiddenCursor;   ///< As X11 doesn't provide cursor hidding, we must create a transparent one
+    ::Cursor   m_hiddenCursor;   ///< As X11 doesn't provide cursor hidding, we must create a transparent one


s/hidding/hiding

mantognini · 2017-04-26T19:06:45Z

I've fixed the type and rebased. Let me know if there's anything else!

eXpl0it3r · 2017-06-03T18:48:01Z

I tested this change on Windows 10 with the small testing code from this gist.

Everything seems to work fine. All the system cursors are showing and my custom cursor are working. During testing I came up with two questions:

When you create a window, the cursor is set to the system arrow cursor, but once you start using an sf::Cursor is there a way to reset the default cursor? Or do you now have to keep sf::Cursor alive throughout the whole application?
The wait cursor (and maybe others) are usually animated cursors (spinning circle, hour glass, etc.), but with SFML the cursors are just static. Has this been done just for consistency among platforms? Or were there other reasons? What would it take to make them animated?

mantognini · 2017-06-05T12:36:49Z

Thanks for giving it a shot! :-)

When you create a window, the cursor is set to the system arrow cursor, but once you start using an sf::Cursor is there a way to reset the default cursor? Or do you now have to keep sf::Cursor alive throughout the whole application?

While defined as "undefined behaviour", if you reset the cursor to the default arrow one, I think all implementation will survive the destruction of the Cursor instance. This is not perfect, but could easily be improved with the help of shared_ptr -- I just don't think the investment is worth it for SFML 2.x branch to implement something similar.

The wait cursor (and maybe others) are usually animated cursors (spinning circle, hour glass, etc.), but with SFML the cursors are just static. Has this been done just for consistency among platforms? Or were there other reasons? What would it take to make them animated?

This wasn't implemented intentionally, but supporting it is tricky. On Linux, it would be a pain in the neck to do it from what I recall (well, like anything I should say...). With Win32, it should be easier to do it, I guess, but I don't know how to get the right resource for the animated cursor. And I think this is a feature not (publicly) exposed on Mac.

eXpl0it3r · 2017-06-05T12:41:08Z

Thanks for clarifying.

I think both points are valid discussions we should come back to at one point, but as a first step, I think this is ready to be merged.

mantognini · 2017-06-05T12:48:12Z

I think both points are valid discussions we should come back to at one point

Yes, definitely.

…pe or custom bitmap image.

binary1248 mentioned this pull request Mar 15, 2015

XCB setMouseCursor #784

Closed

binary1248 added feature m:sfml-window labels Mar 15, 2015

binary1248 self-assigned this Mar 15, 2015

binary1248 added this to the 2.4 milestone Mar 29, 2015

binary1248 removed their assignment Apr 16, 2015

Bromeon reviewed Sep 16, 2015
View reviewed changes

mantognini force-pushed the feature/set_cursor branch from 8c31913 to f2187fa Compare March 9, 2017 17:11

mantognini force-pushed the feature/set_cursor branch from 1e5d1af to 9a9acf7 Compare March 26, 2017 20:15

eXpl0it3r requested changes Apr 4, 2017

View reviewed changes

mantognini previously requested changes Apr 4, 2017

View reviewed changes

mantognini requested changes Apr 5, 2017

View reviewed changes

eXpl0it3r moved this from Requires Adjustments to Review & Testing in SFML 2.5.0 Apr 25, 2017

mantognini force-pushed the feature/set_cursor branch from 9a9acf7 to 6ba2bb1 Compare April 26, 2017 19:05

mantognini approved these changes Apr 26, 2017

View reviewed changes

eXpl0it3r approved these changes Jun 3, 2017

View reviewed changes

eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Jun 5, 2017

mantognini added s:accepted and removed s:undecided labels Jul 1, 2017

binary1248 and others added 5 commits July 10, 2017 17:13

Implemented methods to set the displayed cursor to a system cursor ty…

6f5a7d1

…pe or custom bitmap image.

Rewrote Cursor with new API and OS X implementation

f65459d

Added Win32 implementation

b0b1f13

Added "not supported" implementation for iOS and Android

219c14b

Added X11 implementation

34ea68b

eXpl0it3r force-pushed the feature/set_cursor branch from 6ba2bb1 to 34ea68b Compare July 10, 2017 15:13

eXpl0it3r merged commit 34ea68b into master Jul 18, 2017

eXpl0it3r moved this from Ready to Merged in SFML 2.5.0 Jul 18, 2017

eXpl0it3r deleted the feature/set_cursor branch July 18, 2017 10:05

mantognini mentioned this pull request Apr 2, 2018

Missing cursor types on macos #1401

Closed

mantognini mentioned this pull request Aug 21, 2020

Allow setting cursors with colors on Linux #1693

Closed

Added methods to set the cursor type/image #827

Added methods to set the cursor type/image #827

Conversation

binary1248 commented Mar 15, 2015

fundies commented Mar 15, 2015

LaurentGomila commented Mar 15, 2015

MarioLiebisch commented Mar 15, 2015

binary1248 commented Mar 15, 2015

LaurentGomila commented Mar 15, 2015

MarioLiebisch commented Mar 15, 2015

mantognini commented Mar 15, 2015

binary1248 commented Mar 15, 2015

LaurentGomila commented Mar 15, 2015

mantognini commented Mar 15, 2015

fundies commented Mar 15, 2015

mantognini commented Mar 15, 2015

binary1248 commented Mar 15, 2015

mantognini commented Mar 17, 2015

LaurentGomila commented Mar 17, 2015

mantognini commented Mar 17, 2015

MarioLiebisch commented Mar 17, 2015

binary1248 commented Mar 17, 2015

mantognini commented Mar 17, 2015

eXpl0it3r commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Sep 16, 2015

LaurentGomila commented Sep 17, 2015

Bromeon commented Sep 17, 2015

mantognini commented Mar 5, 2017

eXpl0it3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantognini Apr 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantognini left a comment

Choose a reason for hiding this comment

mantognini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantognini commented Apr 26, 2017

eXpl0it3r commented Jun 3, 2017

mantognini commented Jun 5, 2017

eXpl0it3r commented Jun 5, 2017

mantognini commented Jun 5, 2017

mantognini Apr 4, 2017 •

edited