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
set mouse cursor using a Cursor object #2841
Conversation
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. |
I'd actually go so far and put that stuff on Graphics and not Input, and
|
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. |
I don't have a strong preference on input versus graphics. |
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
Would become
Or (if not checking platform beforehand)
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? |
I wonder if resetting to the default cursor shouldn't be a function in Input instead of having to create a |
I prefer 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. |
Sorry, forgot to comment after my last commit So what changes from actual API:
The following code in 1.5:
Now becomes:
or just:
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) |
I just ran across the bug this PR fixes, and I suppose there's no decent workaround without this PR in the current release? |
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 |
What's missing on this PR to get it merged? We found this problem today as well... |
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... |
Looks fine, just needs a rebase to 1 commit and to be tested on all major OS. |
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! |
Sounds good. Just anyone that can, I've run on Windows and ubuntu, has anyone covered mac side of things? |
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. |
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) |
Awesome, I'll wait for you to do it then 😃 |
The rebase was a bit harder than I thought, I think I messed up some commits a few months ago Tested on (I let the test run a few minutes to make sure everything fine on all platforms)
|
Forgot to reply, but I haven't found any problems with it either. |
Who wants to review and merge this, taking all responsibility for it working correctly? Could also use a blog post on any breaking changes. |
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. |
We can also support custom cursors on the gwt-backend. Change in GwtGraphics.java
to
and add
|
Lets do it! |
set mouse cursor using a Cursor object
PR welcome for gwt support @intrigus |
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.