[PATCH] Hardware cursors on Linux

[Philip Taylor]

It looks like it should be possible to do full 32-bit RGBA cursors on Linux with XcursorImageCreate, kind of like ​what Spring does. If that works, we should do it instead of using the OpenGL-drawn cursor.

See sys_cursor_* in lib/sysdep/os/unix/unix.cpp. Probably need to move that code into os/osx/osx.cpp, then add Xcursor-based versions of the functions into os/linux/linux.cpp. (See also os/win/wcursor.cpp for comparison). Also will need to change lib/res/graphics/cursor.cpp to define ALLOW_SYS_CURSOR 1 on OS_LINUX.

[historic_bruno]

Replying to spledger:

Hi, any progress on this? :)

[bruno]

Note that having hardware cursor's is the difference between the game being playable or not on many linux systems. The game currently isn't playable on an rv530 due to lagging cursor movement. With hardware cursors I think I'll be able to play this on my rv280.

[Jonathan Waller]

I agree, it makes a really big difference. I am increased the priority accordingly.

[Zsolt Dollenstein]

I'll try my hand at this if no takers

[Zsolt Dollenstein]

This is my first attempt at this, please review - especially the changes in lua (I'm a total noob w/ premake) and error handling.

I'm not able to build an OSX version and don't know a thing about X11 on macs, so if someone could give me a hand with that I would appreciate it :)

I hope it's ok to put this in the Alpha 10 Milestone as it's a fairly trivial change

[bruno]

Thanks. I'll try this out this weekend.

[historic_bruno]

Replying to zsol:

I'm not able to build an OSX version and don't know a thing about X11 on macs, so if someone could give me a hand with that I would appreciate it :)

I'll test on OS X and if it works, that's fine, otherwise we can do what the ticket description suggests.

[bruno]

I tried this under Fedora 18 (rawhide) on an rv280 and didn't see a noticeable change. I tried setting both nohwcursor = false and nohwcursor = true.

[Zsolt Dollenstein]

Replying to bruno:

I tried this under Fedora 18 (rawhide) on an rv280 and didn't see a noticeable change. I tried setting both nohwcursor = false and nohwcursor = true.

Right now those options don't affect the behavior (it always uses the new Xcursor implementation). I'll come up with a way to make it configurable tomorrow.

Replying to historic_bruno:

I'll test on OS X and if it works, that's fine, otherwise we can do what the ticket description suggests.

I'm not sure this will even compile properly on osx. If so, can you please try moving lines 46-48 of unix.cpp outside of the #ifdef ?

[Zsolt Dollenstein]

work in progress - should work on unix (for real)

[Zsolt Dollenstein]

I forgot to change cursor.cpp so system cursors were only used on windows. Uploaded a new patch that fixes this, and also moves lines 46-48 of unix.cpp outside of the #ifdef

Let me know if this works for you, I'll probably be able to test this tomorrow on a lower spec machine (on which I can notice cursor lag)

[Jonathan Waller]

I just tried this out, we definitely need this in Alpha 10, it is great.

There is a slight graphical glitch on my system, something to do with transparency so I get a white bar on the right hand side of the default cursor and most of what should be transparency on the treasure cursor is white. I am using the open source ATI drivers.

OpenGL renderer string: Gallium 0.4 on AMD RV770 OpenGL version string: 2.1 Mesa 7.11 OpenGL shading language version string: 1.20

I haven't been able to persuade my PC to take a screenshot of this.

[bruno]

I tried the new patch out and it clearly works. There is still a lag for the effects applied using the cursor, but that is easier to deal without having the cursor position also lagging. There is a glitch in the main cursor though. I have a white bar on the right side of the cursor. The special cursors looked OK though. My display has a 5:4 aspect ratio, in case that makes a difference.

[Zsolt Dollenstein]

Faster and potentially unsafe version

[Zsolt Dollenstein]

Unfortunately I'm not able to reproduce the white bars, but it seems the common denominator here is ATI (all my machines are nvidia)...

This latest version gets rid of some extra work that was done when switching to a new cursor, it may reduce the lag bruno was experiencing. It's not impossible that this may also help with the graphical glitch too but it's just a shot in the dark...

[bruno]

The lag I was seeing after the second patch wasn't directly related to the cursor. An rv280 is a pretty old card and I only get a few frames per second. So while the cursor moves smoothly with that patch, the background isn't always up to date (as is expected). This is still much better than having neither the cursor or the background up to date. I can do things like grab a group of units without waiting for the box to draw, because I can see where the mouse really is when I click.

[bruno]

I didn't see any noticeable difference between the second and third patches.

[Jonathan Waller]

Philip has discovered that X expects a cursor with premultiplied alpha which is what is causing the incorrect transparency behavior.

[historic_bruno]

The latest patch will compile on OS X with minor changes, but it seems ​SDL_GetWMInfo is not implemented (returns 0). Philip made a good point that X11 is not even used by SDL on OS X, so this just won't work.

With that in mind:

• We can make a new ticket for OS X hardware cursors (possibly using ​NSCursor).
• Should probably move the new hardware cursor implementation to somewhere inside sysdep/os/unix/x because it depends on X11. Then move the old stub functions into android.cpp and osx.cpp (I'm generally not a fan of messy #ifdefs when they can be avoided).
• Cursor.cpp line 42: Need to modify logic to make sure the hardware cursors are not used on Android or OS X, but they should be OK on other X11-based Unices, for example Linux or FreeBSD.
• Replace not with ! which is far more conventional in C++.
• SDL_GetWMInfo returns 1 if it succeeds, 0 if not implemented and -1 if implemented but it failed; but all non-zero integers will test as true, so you need to explicitly test for 1. Maybe using debug_printf would be helpful for more informative failure messages.
• premake4.lua changes indeed don't look 100% correct, I think:
  • line 646 isn't needed (Xcursor will get added to used_extern_libs instead)
  • line 713 needs to exclude Xcursor on OS X
  • line 801 needs to exclude Xcursor on Android, so it can be combined around line 770.

[Zsolt Dollenstein]

Added a new patch that incorporates all of the above.

[bruno]

I tested out the latest patch. It didn't apply cleanly against alpha9, but the problem was in the osx section. The issue with a white bar in the cursor was fixed. I also confirmed that the nohwcursor configuration setting worked.

[Zsolt Dollenstein]

Replying to bruno:

I tested out the latest patch. It didn't apply cleanly against alpha9, but the problem was in the osx section.

That's because the patch is against svn HEAD and there's been a change in osx.cpp since alpha9.

[Jonathan Waller]

This patch breaks the simulation test in atlas for me.

[Zsolt Dollenstein]

Replying to quantumstate:

This patch breaks the simulation test in atlas for me.

It looks like SDL_GetWMInfo returns 0 for the window id in atlas. However, there is also an fswindow field in WMInfo which is supposed to tell the full screen window id (this is set in the case of atlas).

I guess it would be easy to fix the code to use fswindow if window == 0, but I don't really understand why SDL is screwing with us like this.

[Philip Taylor]

Some random things:

• premake4.lua: the change on line 768 should be unnecessary, since the linker flags are added via the used_extern_libs.
• extern_libs4.lua: all the existing defined names are lowercase, so the Xcursor = {...} should be xcursor = {...} for consistency.
• extern_libs4.lua: whitespace is wrong (unwanted space character before "link_settings" and "end", and too many tabs for the code between them).
• android.cpp, osx.cpp: too many tabs inside the function definitions.
• android.cpp: should include lib/external_libraries/libsdl.h, instead of directly including SDL.h.
• When nohwcursor is set, there's a 1x1-pixel black square following the mouse, which I think is created by sys_cursor_create_empty. The cursor should either be hidden entirely if that's possible, or it should be a 0x0 cursor if that's allowed, or it should be 1x1 and transparent.

x.cpp:

• Should include Xcursor/Xcursor.h, not X11/Xcursor/Xcursor.h, to be consistent with the other includes that are relative to X11/.
• Functions that are only used inside the current .cpp file should be declared with static (to avoid unnecessary external linkage).
• get_wminfo shouldn't be inline (we almost never use explicit inlines - the compiler can decide whether to inline or not).
• reinterpret_cast<XcursorPixel*>(bgra_img) leads to a potential aliasing violation - there's no guarantee that the input data is a type that's compatible with XcursorPixel* so it's technically not safe for std::transform to read/write it as that type.
• bgra_img shouldn't get modified, since the code that calls sys_cursor_create passes a pointer to data that could be reused by other code later.
• I think it'd be best to get rid of the std::transform, and instead (u8*)malloc(w*h*4) a new buffer and just do an explicit loop to swap-and-copy bytes from the old bgra_img into the new buffer, which should avoid those problems.
• Might XcursorImageCreate or XcursorImageLoadCursor ever return NULL? If so, should detect those cases and return ERR::FAIL.

[Zsolt Dollenstein]

Replying to Philip:

Wow, I didn't realize there could be so many issues with this patch :) I have attached a new one that should address most of these. I still need to check out the nohwcursor case.

I don't agree, however with your points about bgra_img. First of all, bgra_img doesn't get modified. What std::transform does is exactly what you suggested: it loops over (the const) bgra_img, calls cursor_pixel_to_x11_format for every 4-byte chunk and stores the result in the newly allocated image->pixels buffer.

I also think the reinterpret_cast is not unreasonable, since bgra_img's name implies that it's compatible with XcursorPixel and what the cursor_pixel_to_x11_format transformation function requires.

[Philip Taylor]

Replying to zsol:

bgra_img doesn't get modified.

Ah, sorry, I got mixed up when reading the code - that seems fine then.

I also think the reinterpret_cast is not unreasonable, since bgra_img's name implies that it's compatible with XcursorPixel and what the cursor_pixel_to_x11_format transformation function requires.

Oops, looks like I got mixed up here too. The hypothetical danger was that someone could write

unsigned short data[] = { 0x1122, 0x3344 };
sys_cursor_create(1, 1, static_cast<void*>(data), ...);

so the data is declared with type short, and if you tried to read it by dereferencing a pointer to type XcursorPixel (unsigned int), the C++ standard says that's undefined behaviour because it violates the ​aliasing requirements. But you're passing it to cursor_pixel_to_x11_format as a reference and casting to unsigned char* before dereferencing, which is defined as safe, so that's okay. All the casting is a bit confusing but not actually illegal.

If you're not annoyed by insignificant pedantic suggestions, there's a few more:

• On the subject of aliasing, cursor_pixel_to_x11_format is slightly inefficient in using *a multiple times (instead of copying it into a local variable once) - the compiler can't tell that dst doesn't overlap with a, so it'll have to re-read *a from memory every time just in case the assignment to *dst changes it. But this code is going to be so fast anyway that performance is irrelevant, so don't worry about that :-)
• sizeof(unsigned char) is guaranteed to be 1 in C++, so there's no point in using it here.
• Per coding conventions, code in lib/ (though not elsewhere) should use "if(...)" instead of "if (...)"
• Use u8 instead of unsigned char, to be consistent with the (undocumented) type conventions in lib/

[Zsolt Dollenstein]

Hardware cursors on unix (with fix for atlas crash)

[Zsolt Dollenstein]

Fixed all of the above except sizeof(unsigned char) - now sizeof(u8) - because I think it's slightly clearer (no overhead whatsoever as it's a compile time constant).

I also cleared up x.cpp to conform to the above comments :)

[Zsolt Dollenstein]

Attached a little treat for osx users :) osx TODOs:

• error handling
• nohwcursor use case

[historic_bruno]

Have you tested the OS X code?

I haven't, there's only one thing I've been thinking about, we are probably relying on external libs linking to certain frameworks we need like Foundation and now AppKit. So I think we should explicitly link at least the Cocoa framework (see ​this table) in premake4.lua in the main EXE setup function. ApplicationServices is another framework we use. Otherwise if wxWidgets or SDL change their dependencies, we might have link errors (and also I've seen on Linux that GCC has become more demanding and requires these explicit links). Does that make sense?

Incidentally I'm also working on something that uses AppKit, one annoyance I've encountered is that ​NSAlert is not thread-safe, in fact it seems much of AppKit is not thread-safe and must be called only on the main UI thread. This won't cause a problem in normal game mode because the engine is always running on the main thread, but in Atlas, the UI is the main thread and the engine is actually a secondary thread. So I'm wondering will this code cause thread safety issues in Atlas? (It seems it should not even be used in Atlas but I just want to make sure)

[Zsolt Dollenstein]

Replying to historic_bruno:

Have you tested the OS X code?

Yes, briefly. It works ingame.

I haven't, there's only one thing I've been thinking about, we are probably relying on external libs linking to certain frameworks we need like Foundation and now AppKit. So I think we should explicitly link at least the Cocoa framework (see ​this table) in premake4.lua in the main EXE setup function. ApplicationServices is another framework we use. Otherwise if wxWidgets or SDL change their dependencies, we might have link errors (and also I've seen on Linux that GCC has become more demanding and requires these explicit links). Does that make sense?

Yes, it does. I was thinking about the same thing, but since I don't know the first thing about OSX and its frameworks, I'll go with whatever you suggest :)

Incidentally I'm also working on something that uses AppKit, one annoyance I've encountered is that ​NSAlert is not thread-safe, in fact it seems much of AppKit is not thread-safe and must be called only on the main UI thread. This won't cause a problem in normal game mode because the engine is always running on the main thread, but in Atlas, the UI is the main thread and the engine is actually a secondary thread. So I'm wondering will this code cause thread safety issues in Atlas? (It seems it should not even be used in Atlas but I just want to make sure)

Unfortunately it does get used in atlas when you start a simulation test. Unfortunately #2 I was not able to build Atlas on my machine (haven't given it much effort though), so I have no idea if the patch even works at all for it.

[historic_bruno]

OK, I'll test on OS X later since I have Atlas working. My strong suspicion is that it's not thread safe :(

I think your patch will leak the NSBitmapImageRep and NSImage objects because they get ​allocated but not ​released. I'm not 100% comfortable with Cocoa/Obj-C but I think you should release them as soon as the reference is no longer needed (a quick search confirms this usage, e.g. ​here).

There is a space after the ":" on line 49 of osx_sys_cursor.mm, but not any of the others.

[historic_bruno]

I tested on OS X Lion. First of all, this makes a huge difference in playability! The cursor movement is so smooth now :)

It compiled fine and ran in both the game and Atlas. I didn't notice any warnings in the console about NSCursor/AppKit thread safety when using Atlas' simulation test. That doesn't mean there are no thread safety issues, it just means no warning was given, so I'm inclined not to worry about it yet (or maybe leave it as a TODO in the code).

The problem I did notice is that the game cursor reverts to the system cursor when it leaves the window (in windowed mode), but it doesn't always change back when re-entering. Sometimes you have to click in the session UI to force the cursor to change. I don't know if the same problem exists on Linux, that should be tested.

[Zsolt Dollenstein]

Latest patch with support for linux and osx

[Zsolt Dollenstein]

Please review the latest version :) Changes since last patch:

• fixed leaking NSImage and NSBitmapImageRep
• added error handling for osx
• implemented sys_cursor_create_empty (nohwcursor = true)
• added TODO note mentioning possible thread-safety issue
• main exe now explicitly links against AppKit framework on osx (this is the only one I'm using in these patches)

Linux side of things are unchanged. I don't think we can do anything about the problem historic_bruno notes in comment:35, the focus is simply not on the application, so sys_cursor_* functions are not even called (until you click the UI). I believe this can be reproduced on any OS with click-to-focus mouse policy (but I haven't actually verified this)

[ben]

In 11594:

Fixes bug detecting mouse focus change. Refs #748.

[historic_bruno]

Actually there was a bug, we treated ​SDL_ActiveEvent.state as if it were a single value when it's really a bitfield. My guess is on OS X and possibly Linux, two or more of the bit flags were set each time focus changed causing the equality check to fail and the cursor to not be reset.

There is a slight delay switching cursors, but it does work now :) I'm going to clean up a few things and commit your patch.

[Zsolt Dollenstein]

wow, nice catch. Thanks! :)

[ben]

In 11596:

Adds hardware cursors for Linux and OS X. Requires libxcursor on Linux. Fixes #748.
Adds explicit links to frameworks we need on OS X.

[historic_bruno]

Damn, I forgot to mention you in the commit message. Ah well, we will definitely add you to the game's contributors nonetheless, this is an awesome patch :)

Now we just have to update BuildInstructions to mention libxcursor-dev (at least it was needed on my Ubuntu test machine).

[fabio]

I tried it on Ubuntu 12.04, libxcursor-dev is installed but I get a link error (see error.txt).

[Zsolt Dollenstein]

You also need libxcursor1

[historic_bruno]

Replying to fabio:

I tried it on Ubuntu 12.04, libxcursor-dev is installed but I get a link error (see error.txt).

Hmm, even after running ./update-workspaces.sh?

[fabio]

OK, I indeed tried ./update-workspaces.sh and it works fine.

No more cursor lag, great!

[Kieran P]

Seems like the OSX part was missed :-( Was about to give this a go but can't :-(

[Kieran P]

Sorry, my mistake. It was there. Awesome work :-)

[Kieran P]

Absolutely brilliant. Not only has it fixed the slow cursor speed, but it's also fixed the bug where the cursor would disappear when switching out and back to the game, and ALSO seems to have enabled icons for the cursor (things like wood, cherrys, garrison etc). I don't recall them working before the patch. Superb work! Can't wait to see what you work on next :-)