set mouse cursor using a Cursor object

[haedri]

This pull request is to solve issue #2839

Basically problem is that currently calling Gdx.input.setCursorImage will create a new (lwjgl) Cursor object everytime, even if pixmaps are identical + it will make some calculations to process the Pixmap.
This is ok if only a few setCursorImage call are made, but if the application requires to cycle a lot through a few cursors then it will eventually crash on Windows (invisible cursor) and also use unecessary memory + cpu.

This pull request adds a new Cursor object that can be created by calling Gdx.input.newCursor, and set anytime using Gdx.input.setCursor, when using this method no other object are created when calling Gdx.input.setCursor. It kinda provides a way for the user to use a cache for cursor creation.

This is my very first pull request (first time using commit, push and pull request actually) and my first contribution to Libgdx. So if I made anything wrong don't hesitate to tell me, I will do my best to correct this.

There are two things I am not sure about:
1- I kept the old Gdx.input.setCursorImage for compatibility reasons, it will call newCursor/setCursor, maybe it should be noted deprecated or something...
2- The Gdx.input.setCursor method is not necessary as the Cursor object has a setCursor() function, I was thinking of removing it, but lept it to match current setCursorImage method and to provide an easy way to revert to default cursor (other way is to call newCursor with a null pixmap and use this cursor next). It's 50/50, so if you want me to remove it I can easily understand.

[NathanSweet]

Looks ok to me. Usually I prefer just changing the API rather than leaving behind old methods, but I don't feel to strongly about it for this PR at the moment.

[badlogic]

I'd actually go so far and put that stuff on Graphics and not Input, and
kill the old methods.
On Feb 14, 2015 12:25 AM, "Nathan Sweet" notifications@github.com wrote:

Looks ok to me. Usually I prefer just changing the API rather than leaving
behind old methods, but I don't feel to strongly about it for this PR at
the moment.

—
Reply to this email directly or view it on GitHub
#2841 (comment).

[haedri]

Moving it to graphics could make sense since it's a graphical stuff, only (small) problem is that everything related to mouse is in input, but setting a cursor is not "input" per se

So if you are ok to do this kind of refactoring (and losing compatibility with previous version), then I can move to graphics the newCursor function and only keep this one.

[NathanSweet]

I don't have a strong preference on input versus graphics.

[haedri]

Here is a proposal: the old function Input.setCursorImage is removed, and there is just a new function in Graphics called newCursor.

It might be a little cleaner, but it looses compatibility with old code.

What was

Gdx.input.setCursorImage(pixmap, 16, 16);

Would become

Gdx.graphics.newCursor(pixmap, 16, 16).setSystemCursor();

Or (if not checking platform beforehand)

Cursor cursor = Gdx.graphics.newCursor(pixmap, 16, 16);
if (cursor != null)
    cursor.setSystemCursor();

In my opinion we shouldn't remove previous api function, so I can put it back but it will just call the new one in Graphics (and maybe mark it as deprecated and removed in a future api version). What do you think?

[Zomby2D]

I wonder if resetting to the default cursor shouldn't be a function in Input instead of having to create a Cursor with a null Pixmap. Personally I would have kept the setCursor function in Input and just moved the Cursor part to Graphics.

[NathanSweet]

I prefer newCursor and setCursor over the Cursor knowing how to set itself.

Don't deprecate methods, just make the API changes. Otherwise we have junk laying around that needs to be cleaned up later, and it probably won't be.

[haedri]

Sorry, forgot to comment after my last commit

So what changes from actual API:

• Everything related to cursor has been moved to Gdx.graphics (as it's graphical stuff anyway)
• setCursorImage has been removed
• To create a "cursor image" one must use Gdx.graphics.newCursor(pixmap, x, y)
• To set a cursor image Gdx.graphics.setCursor(cursor)
• To revert to default cursor Gdx.graphics.setCursor(null)

The following code in 1.5:

Gdx.input.setCursorImage(pixmap, 16, 16);

Now becomes:

Cursor cursor = Gdx.graphics.newCursor(pixmap, 16, 16);
Gdx.graphics.setCursor(cursor);

or just:

Gdx.graphics.setCursor(Gdx.graphics.newCursor(pixmap, 16, 16));

If someone wants to change cursor multiple times then first syntax is best as he can reuse the cursor object. Second syntax will behave like current implementation (can loose cursor under Windows, and creates temporary objects)

[Pigmassacre]

I just ran across the bug this PR fixes, and I suppose there's no decent workaround without this PR in the current release?

[haedri]

Just updated a bit the javadoc, I had a problem on Linux + Intel card + vsync disabled. http://www.badlogicgames.com/forum/viewtopic.php?f=11&t=18997&p=79785

Basically, don't change the cursor too often in a small time frame, system or driver might not like that

[Arcnor]

What's missing on this PR to get it merged? We found this problem today as well...

[Arcnor]

I don't want to be annoying, but we really need this fixed, and we are open to finish the work ourselves, if there is anything to fix...

[Tom-Ski]

Looks fine, just needs a rebase to 1 commit and to be tested on all major OS.

[Arcnor]

If it helps, we've published a game on Steam with this patch applied, and so far, so good. Having the cursor disappear randomly on Windows was a real problem...

If @haedri is busy, I'll try to do the rebase. Not sure how to do the testing (besides what we're actually doing, of course) or even if somebody outside the team should do it :)

Thanks for the reply @Tom-Ski!

[Tom-Ski]

Sounds good.

Just anyone that can, I've run on Windows and ubuntu, has anyone covered mac side of things?

[Arcnor]

I'll let you know after I do more testing just in case, but I've been running for a while with the patch with no obvious bad effects.

[haedri]

I can't do the rebase right now (at work), but I can try in a few hours (I never did a rebase before, my understanding is that it's some sort of merge, so shouldn't be too hard)
For my game I've tested Windows (xp 32bits , 7 64bits) / Linux (ubuntu 14.04 64bits) / Mac(Yosemite), no regression so far

[Arcnor]

Awesome, I'll wait for you to do it then 😃

[haedri]

The rebase was a bit harder than I thought, I think I messed up some commits a few months ago
So I did a full reset and added back the modifications (which closed the PR, sorry for that), it seems a lot cleaner now
I have also added a simple test case that switches between two cursors every frame (and displays a third one on touch)

Tested on (I let the test run a few minutes to make sure everything fine on all platforms)

• Mac OS (Mac mini, Yosemite)
• Linux 64 bits (Xubuntu 14.14)
• Windows XP 32 bits (VM Virtualbox)
• Windows 7 64 bits

[Arcnor]

Forgot to reply, but I haven't found any problems with it either.

[NathanSweet]

Who wants to review and merge this, taking all responsibility for it working correctly? Could also use a blog post on any breaking changes.

[Arcnor]

Can we merge this already? Not sure if @NathanSweet was asking for someone inside LibGDX to take responsibility for it, or for someone outside, but in any case we've been running with this patch in our released game with no reports of it breaking anything mouse related.

[intrigus]

We can also support custom cursors on the gwt-backend.

Change in GwtGraphics.java

    @Override
    public Cursor newCursor (Pixmap pixmap, int xHotspot, int yHotspot) {
        return null;
    }

    @Override
    public void setCursor (Cursor cursor) {
    }

to

    @Override
    public Cursor newCursor (Pixmap pixmap, int xHotspot, int yHotspot) {
        return new GwtCursor(pixmap, xHotspot, yHotspot);
    }

    @Override
    public void setCursor (Cursor cursor) {
        if (cursor == null) {
            GwtCursor.resetCursor();
        } else {
            cursor.setSystemCursor();
        }
    }

and add

/*******************************************************************************
 * Copyright 2011 See AUTHORS file.
 * 
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 * 
 *   http://www.apache.org/licenses/LICENSE-2.0
 * 
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 ******************************************************************************/

package com.badlogic.gdx.backends.gwt;

import com.badlogic.gdx.Gdx;
import com.badlogic.gdx.graphics.Cursor;
import com.badlogic.gdx.graphics.Pixmap;
import com.badlogic.gdx.utils.GdxRuntimeException;
import com.google.gwt.dom.client.CanvasElement;

public class GwtCursor implements Cursor {
    protected String cssCursorProperty;

    public GwtCursor (Pixmap pixmap, int xHotspot, int yHotspot) {
        if (pixmap == null) {
            this.cssCursorProperty = "auto";
            return;
        }

        if ((pixmap.getWidth() & (pixmap.getWidth() - 1)) != 0) {
            throw new GdxRuntimeException("Cursor image pixmap width of " + pixmap.getWidth()
                + " is not a power-of-two greater than zero.");
        }

        if ((pixmap.getHeight() & (pixmap.getHeight() - 1)) != 0) {
            throw new GdxRuntimeException("Cursor image pixmap height of " + pixmap.getHeight()
                + " is not a power-of-two greater than zero.");
        }

        if (xHotspot < 0 || xHotspot >= pixmap.getWidth()) {
            throw new GdxRuntimeException("xHotspot coordinate of " + xHotspot + " is not within image width bounds: [0, "
                + pixmap.getWidth() + ").");
        }

        if (yHotspot < 0 || yHotspot >= pixmap.getHeight()) {
            throw new GdxRuntimeException("yHotspot coordinate of " + yHotspot + " is not within image height bounds: [0, "
                + pixmap.getHeight() + ").");
        }
        StringBuilder stringBuilder = new StringBuilder("url('");
        stringBuilder.append(pixmap.getCanvasElement().toDataUrl("image/png"));
        stringBuilder.append("')");
        stringBuilder.append(xHotspot);
        stringBuilder.append(" ");
        stringBuilder.append(yHotspot);
        stringBuilder.append(",auto");
        this.cssCursorProperty = stringBuilder.toString();
    }

    @Override
    public void setSystemCursor () {
        ((GwtApplication)Gdx.app).graphics.canvas.getStyle().setProperty("cursor", cssCursorProperty);
    }

    public static void resetCursor () {
        ((GwtApplication)Gdx.app).graphics.canvas.getStyle().setProperty("cursor", "auto");
    }
}
    }

    @Override
    public void setSystemCursor () {
        ((GwtApplication)Gdx.app).graphics.canvas.getStyle().setProperty("cursor", cssCursorProperty);
    }

    public static void resetCursor () {
        ((GwtApplication)Gdx.app).graphics.canvas.getStyle().setProperty("cursor", "auto");
    }
}

[Tom-Ski]

Lets do it!

[Tom-Ski]

PR welcome for gwt support @intrigus