Patch: WindowProperties.setCursorFilename() does not work on Linux

[Maxwell J. Koo]

On windows, one is able to set a custom cursor by using WindowProperties.setCursorFilename() with the path of a windows cursor file. This does not work on linux/x11 in the current CVS build of panda3d. As a result, hacky workarounds such as rendering geometry to follow the mouse pointer are necessary to use custom cursors in panda3d programs.

This patch fixes this issue, by using the Xcursor library to properly set the cursor under x11. Note that this will NOT fix the problem on mac osx systems, but does work under linux.

Apparently this has been recognized as a bug for some time but was never addressed. See http://www.panda3d.org/forums/viewtopic.php?p=10176 .

Tested under Ubuntu Lucid 10.04 32 bit, CVS build of panda3d, built with makepanda.

I will upload sample code that demonstrates the issue as an attachment.

[Maxwell J. Koo (mjkoo)]

Sample code showing the issue, without patch the mouse pointer is inherited from the root window, with patch it becomes a crosshair.

[Maxwell J. Koo (mjkoo)]

Crosshair resource for the attached sample code

[rdb (rdb)]

Awesome, thank you very much! I've committed your patch. It'll be picked up in the next release of Panda3D, and the daily buildbots will also pick it up with the next build.
I've also committed the proper rules to the ppremake scripts to build this correctly using ppremake.

The only concern I'd have is that it wouldn't work the same across platforms, e.g. it'd require a different cursor format on Windows and Linux. But I'm not sure if we can achieve that at all, and I'm happy enough with having custom cursor support on X11 now.

Thank you so much for providing this patch!

[Maxwell J. Koo (mjkoo)]

No problem, my pleasure. Thanks to you guys for putting out an awesome
framework!

The same thought did occur to me, an option I was thinking about would be to
write a function to parse a windows .ico into a XcursorImage struct and have
that as another fallback method in case the regular load from file failed.
If that is something you/the project would be interested in, I'm on winter
break now and fairly bored so I wouldn't mind at all looking into it.

Thanks,
Max

On Tue, Dec 21, 2010 at 7:40 AM, rdb <email address hidden> wrote:

> Awesome, thank you very much! I've committed your patch. It'll be picked
> up in the next release of Panda3D, and the daily buildbots will also pick it
> up with the next build.
> I've also committed the proper rules to the ppremake scripts to build this
> correctly using ppremake.
>
> The only concern I'd have is that it wouldn't work the same across
> platforms, e.g. it'd require a different cursor format on Windows and
> Linux. But I'm not sure if we can achieve that at all, and I'm happy
> enough with having custom cursor support on X11 now.
>
> Thank you so much for providing this patch!
>
> ** Changed in: panda3d
> Importance: Undecided => Wishlist
>
> ** Changed in: panda3d
> Status: New => Fix Committed
>
> ** Changed in: panda3d
> Milestone: None => 1.7.1
>
> ** Changed in: panda3d
> Assignee: (unassigned) => Maxwell J. Koo (mjkoo)
>
> --
> You received this bug notification because you are a direct subscriber
> of the bug.
> https://bugs.launchpad.net/bugs/692956
>
> Title:
> Patch: WindowProperties.setCursorFilename() does not work on Linux
>
> Status in Panda3D:
> Fix Committed
>
> Bug description:
> On windows, one is able to set a custom cursor by using
> WindowProperties.setCursorFilename() with the path of a windows cursor file.
> This does not work on linux/x11 in the current CVS build of panda3d. As a
> result, hacky workarounds such as rendering geometry to follow the mouse
> pointer are necessary to use custom cursors in panda3d programs.
>
> This patch fixes this issue, by using the Xcursor library to properly set
> the cursor under x11. Note that this will NOT fix the problem on mac osx
> systems, but does work under linux.
>
> Apparently this has been recognized as a bug for some time but was never
> addressed. See http://www.panda3d.org/forums/viewtopic.php?p=10176 .
>
> Tested under Ubuntu Lucid 10.04 32 bit, CVS build of panda3d, built with
> makepanda.
>
> I will upload sample code that demonstrates the issue as an attachment.
>
> To unsubscribe from this bug, go to:
> https://bugs.launchpad.net/panda3d/+bug/692956/+subscribe
>

[Maxwell J. Koo (mjkoo)]

Apologies, one last thing. I forgot to release the Cursor objects in the destructor, small patch to iterate through the map and release them with XFreeCursor(). Wouldn't want to introduce a memory leak...

[David Rose (droklaunchpad)]

This sounds like a great idea to me!

David

On 12/21/10 5:27 AM, Maxwell J. Koo wrote:
> The same thought did occur to me, an option I was thinking about would be to
> write a function to parse a windows .ico into a XcursorImage struct and have
> that as another fallback method in case the regular load from file failed.
> If that is something you/the project would be interested in, I'm on winter
> break now and fairly bored so I wouldn't mind at all looking into it.

[rdb (rdb)]

Great, I've applied the latest patch. Thanks!
We need more people like you. :-)

[Maxwell J. Koo (mjkoo)]

Alright, here we go, rudimentary windows ICO file parser that allows the use of ICO cursors on linux. This means that you can now just create one cursor for windows and it should work on both platforms (but still not on mac). Loads 5 different formats of ICOs.

Known limitations: doesn't support PNG compressed icons, probably can pull in some libpng stuff to get that going, only known to work on the types of ICOs exportable by GIMP but it loaded the bundled panda3d.ico file no problem ;-), leaves the hotspot at 0, 0 if the file is a standard ICO, if its a CUR file (pretty much the same) it will detect this and set the hotspot appropriately.

I tried to keep this as simple as possible, so it all fits in just one extra method in x11GraphicsWindow. If you guys think it should be expanded or refactored some other way I'd be happy to do that too.

To test, just change the cursor file path in the previous example to point to any ico, such as the panda3d ico.

[rdb (rdb)]

Excellent work, many thanks! Sorry that it took me so long to get back to you, I have been busy with other things.

I've committed your patch, but I've made various modifications. First of all, I found it a bit dirty that XcursorFilenameLoadCursor was called even when the file is not an XCursor file, so it now checks the first four bytes of the file to see if it's the right format before trying to load it.

Secondly, the cursors are now loaded through Panda's virtual file system. In the case of an X11 cursor, this is done by wrapping the istream in an XCursorFile struct and setting the appropriate function pointers.
Loading it through the VFS allows loading cursors from multifiles, and allows loading cursors when the application is packed into a .p3d file.

Thirdly, I changed it to prefer the default cursor size (as returned by XcursorGetDefaultSize) when loading an .ico or .cur file.

Fourthly and most importantly, none of the .cur files I found on my system were able to load. They appeared to be 1-bpp cursor files. The code was trying to read a palette of 256 colours even though there were just two.
After failing to find some good specifications on the format, I found several .ico readers on Google CodeSearch, and all of them ignored the colour count specified by the headers and assumed the colour count to be 1 << bpp instead.
So, I've changed the code to this:
  if (bitsPerPixel != 24 && bitsPerPixel != 32) {
    colorCount = 1 << bitsPerPixel;
    palette = new IcoColor[colorCount];
    ico.read(reinterpret_cast<char *>(palette), colorCount * sizeof(IcoColor));
    if (!ico.good()) goto cleanup;
  }
My .cur files load correctly now.

Thanks again for your contributions!

[rdb (rdb)]

On a side note, it's probably a good idea to check where the hot spot is placed when an .ico file is loaded on Windows, to make sure we provide consistent behaviour.

[Maxwell J. Koo (mjkoo)]

Awesome! Sorry about the various bugs, I don't really have a windows machine to test it on so my test set was somewhat limited, but I'm glad to see this works for you.

[rdb (rdb)]

For the record: I just checked in various bugfixes. For instance, because the filename is stored twice in _cursor_filenames, a double free can happen. Furthermore, it didn't check if a cursor was requested when opening a new window, so setting the "cursor-filename" PRC variable didn't work. I can view custom cursors in pview too, now.