-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Added methods to set the cursor type/image #827
Conversation
I don't care about credit, all I wanted was an implementation. Thanks. |
What would be nice now is to get rid of |
@LaurentGomila I like that idea. But before adding/changing something twice, how about creating a
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 |
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.
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.
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.
What existing enum? |
Ah right. So either we do nothing and implement it in SFML 3, or we add |
You misunderstood me. It would be just another resource, representing a cursor by holding/using a
I wrote that line before noticing that this is already in the PR, I just didn't edit it out.
The one introduced by this PR. |
@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. 😉 |
What do the others think of providing a separate @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. |
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. |
Yes, but since setting an icon of a window already provides the code to load a bitmap and that we have As for |
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... |
@fundies This is (or at least should be) already handled by the gist (indirectly) linked in the description. ;-) |
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.
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. 😛 |
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. |
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. |
I guess there would be two constructors in |
Yep, sounds good. :) |
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. |
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. 😉 |
What's the final verdict here? Also the branch needs rebasing. |
include/SFML/Window/Window.hpp
Outdated
Arrow, ///< Arrow cursor (default) | ||
ArrowWait, ///< Busy arrow cursor | ||
Wait, ///< Busy cursor | ||
Text, ///< I-beam, cursor when hovering over a field allowing text entry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that not called "caret"? Or is caret only the blinking thing indicating where text is going to be inserted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the blinking thing indicating where text is going to be inserted
This.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think "Text" is fine as a name. It describes the cursor's purpose pretty well.
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 void setMouseCursor(const Uint8* pixels, Vector2u size, Vector2u hotspot); |
Didn't we want to add |
Sounds good, the question is: what would Also, there was a discussion about a dedicated
|
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 |
8c31913
to
f2187fa
Compare
1e5d1af
to
9a9acf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pure code-reading review. I'll test the implementation later on Windows.
@@ -25,6 +25,7 @@ | |||
#ifndef SFML_CONTEXTSETTINGS_HPP | |||
#define SFML_CONTEXTSETTINGS_HPP | |||
|
|||
#include <SFML/Config.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that some left-over, or why is this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might well be, yes! I'll mark this as a requested change and deal with it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// \return a reference to the OS-specific implementation | ||
/// | ||
//////////////////////////////////////////////////////////// | ||
const priv::CursorImpl& getImpl() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we do this similarly in other classes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean deferring to an Impl class? Yes, we did it for thread, input, sensor, window, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, providing an explicit getImpl()
function for a friended class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// \see sf::Cursor::loadFromPixels | ||
/// | ||
//////////////////////////////////////////////////////////// | ||
void setMouseCursor(const Cursor& cursor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
;-)
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume hotspot is a technical and commonly used term, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct: it's used by Linux, Windows and Apple.
//////////////////////////////////////////////////////////// | ||
bool Cursor::loadFromPixels(const Uint8* pixels, Vector2u size, Vector2u hotspot) | ||
{ | ||
if ((pixels == 0) || (size.x == 0) || (size.y == 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
switch (type) | ||
{ | ||
default: return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
/// \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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Linux only supports monochrome cursors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I could find in the X11 documentation, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
XFreePixmap(m_display, maskPixmap); | ||
|
||
// We assume everything went fine... | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's really no way this function could ever fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
unsigned int shape; | ||
switch (type) | ||
{ | ||
default: return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, should come after the cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same. :-)
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exactly is it ::Cursor
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid a clash with sf::Cursor
and X11 cursor, which lives in the global namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at those lines, I'm pretty sure it's "hiding" vs. "hidding". ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huhu, yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove #include <SFML/Config.hpp>
from ContextSettings.hpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo to fix
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/hidding/hiding
9a9acf7
to
6ba2bb1
Compare
I've fixed the type and rebased. Let me know if there's anything else! |
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:
|
Thanks for giving it a shot! :-)
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
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. |
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. |
Yes, definitely. |
6ba2bb1
to
34ea68b
Compare
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.