Re: winex11.drv: Added missing mouse position clipping. (try 2)
The SetCursorPos() calls hooks on Wine because Wine has big issue - it doesn't filter pointer move messages that were caused by Wine itself.
The first patch (http://source.winehq.org/patches/data/57569) tries to address exactly this. I think XWarpCursor is not a problem, as the event comes asynchronously with some delay. So my patch fixes the easier but worse case, calling hooks directly from SetCursorPos. What's wrong with this, why is it wrong, and do you have a better idea? What else causes Wine to produce extra messages? Also, is there a reason why I shouldn't write a test for this to make the issue better known?
I'm aware that my version causes hooks to be called with clipped coordinates on mouse move, when they should in that case be called with the unclipped ones. This particular change will break all games using dinput when mouse pointer goes outside of virtual desktop.
(This was the elaboration I was asking for.) I didn't know that, and I don't have any such games. Feel free to reject the patch, I'll rewrite it without this bug and in smaller chunks. (It would be helpful if the first two patches were also either accepted or rejected.)
returns unclipped coordinates from GetCursorPos This is what native will do inside hook handler. Add some tests.
That's not true. (See below.)
and even sends incorrect WM_* messages. Tests please.
I've attached a program that outputs coordinates from hookproc and wndproc, both the passed ones and the ones from GetCursorPos. If you try it, you can see that Wine has the following bugs: - incorrect coordinates in some WM:s - incorrect coordinates in some hook calls - incorrect coordinates and missing WM:s after clipping - extra hook calls on SetCursorPos, even if the pos doesn't change - extra WM_MOUSEMOVE after zero-length moves - some other extra hook calls and WM:s due to XWarpPointer The next snippets show the WM bug and how GetCursorPos works inside hook handlers. This first one is from Win2k8 Server. First, the pointer is at (34,34) and clipping is set to (30,30)-(40,40). Then the program calls mouse_event(MOUSEEVENTF_LEFTUP | MOUSEEVENTF_MOVE, 0, 6, 0, 0). 11: hookproc: WM_MOUSEMOVE: (34,40), get = (34,34), injected = 1 11: hookproc: WM_LBUTTONUP: (34,39), get = (34,39), injected = 1 11: wndproc: WM_MOUSEMOVE: (34,39), get = (34,39) 11: wndproc: WM_LBUTTONUP: (34,39), get = (34,39) As you can see, the correct order would be: - call hooks for moving, return old pos from GetCursorPos - clip and store the new position - send the move message using the new position - call hooks and send the button message, both using the clipped position Compare to current Wine output: 11: hookproc: WM_MOUSEMOVE: (34,40), get = (34,34), injected = 1 11: hookproc: WM_LBUTTONUP: (34,40), get = (34,39), injected = 1 11: hookproc: WM_MOUSEMOVE: (34,39), get = (34,39), injected = 0 11: wndproc: WM_MOUSEMOVE: (34,40), get = (34,39) 11: wndproc: WM_LBUTTONUP: (34,40), get = (34,39) 11: wndproc: WM_MOUSEMOVE: (34,39), get = (34,39) Aside of the extra move caused by XWarpPointer, the second hook and both injected messages have incorrect (unclipped) coordinates.
What exactly is wrong with [border scrolling]?
Many games (for example, Baldur's Gate) check for mouse coordinates. If the pointer is outside the virtual desktop, Wine gives negative coordinates and the game doesn't scroll left/up, because x != 0 and y != 0. The same thing happens to right/bottom, of course. Correct clipping fixes it.
Who said that Wine should keep the pointer inside virtual desktop?
Ok, I'll not do it. But don't you think it's a bit confusing if the pointer moves freely but the program thinks it's clipped to an area? My apologies for bringing up something that is actually none of my business, but I think you should pay more attention to the way you write your comments. First, even small positive comments are considered psychologically very important, but you have given none. Second, most of your negative feedback has only stated that the patches are bad and wrong, often without giving much details or any better ideas. That is, the comments haven't been very constructive. Currently your messages look a bit like "f*** off, noob", which hopefully is not what you had in mind. Anyway, this is certainly not a good way to encourage spending time to Wine development. Some (luckily not me) would take it badly and just rm -rf wine-git and never try again, even if they could be a great help with some guidance. So let's all be nice to each other, and everyone will be happier. -- Lauri Kenttä
Lauri Kenttä wrote:
My apologies for bringing up something that is actually none of my business, but I think you should pay more attention to the way you write your comments. First, even small positive comments are considered psychologically very important, but you have given none. Second, most of your negative feedback has only stated that the patches are bad and wrong, often without giving much details or any better ideas. That is, the comments haven't been very constructive. Currently your messages look a bit like "f*** off, noob", which hopefully is not what you had in mind. Anyway, this is certainly not a good way to encourage spending time to Wine development. Some (luckily not me) would take it badly and just rm -rf wine-git and never try again, even if they could be a great help with some guidance. So let's all be nice to each other, and everyone will be happier.
Lauri: Programming can and is brutal. I deal with this daily and I've learned to keep a smile on my face, even when I'm being chastised. Drives folks 'nuts'. However, you have to learn the lesson, even if you win. One thing is that Windows does not trap the mouse and neither should Wine. If you are in a virtual desktop, the mouse should stop at the edge of the window and move to where you re-enter it. This is what you should strive for. If you cannot do this, then step back and take another try at it. I have been working on a fix for ONE function in Richedit for over one and one-half year now and the complaints still come in. However, I take them as constructive hints and work towards fixing and clearing them off. Soon, the patch will be submitted and I will move onto other problems that will rise because of the implementation of this code that are outside the scope of the code. That is the way it is in this business. James McKenzie
James McKenzie wrote:
Programming can and is brutal.
Yes, I know. But programming can also be nice and fun. It will be exactly what the community makes it. I've seen both kinds, and guess which ones have had more active users. This was my point, so do not misunderstand: I'm certainly not saying that bad patches should be accepted. But enough of that, now.
Windows does not trap the mouse and neither should Wine.
I'm not sure what you mean. If you're saying that ClipCursor doesn't actually trap it, please try it out first. As simple as this: #include <stdio.h> #include <windows.h> int main() { RECT rc; SetRect(&rc, 100, 100, 200, 200); ClipCursor(&rc); getchar(); ClipCursor(NULL); return 0; } If you're running Windows on a virtual machine, remember to disable things like "mouse pointer integration" before testing.
If you are in a virtual desktop, the mouse should stop at the edge of the window and move to where you re-enter it.
I think we all agree about that, but GetCursorPos currently doesn't. Also, the last WM_MOUSEMOVE comes from the last position inside the desktop, which probably is not at the edge. But that's something I'll not try to fix. Thanks for your comments! -- Lauri Kenttä
Lauri Kenttä wrote:
James McKenzie wrote:
Programming can and is brutal.
Yes, I know. But programming can also be nice and fun. It will be exactly what the community makes it. I've seen both kinds, and guess which ones have had more active users. This was my point, so do not misunderstand: I'm certainly not saying that bad patches should be accepted.
Been there, done that. It is always desireable to have harmony. However, there will always be the person or persons that will be brutal. That is Vitaly's way and nothing so far has changed. I just accept comments and dismiss the other items. That is best for now.
Windows does not trap the mouse and neither should Wine.
I'm not sure what you mean. What I mean is that windowed games do not trap the mouse at the windows edge and prevent it from leaving the window.
If you're saying that ClipCursor doesn't actually trap it, please try it out first. As simple as this:
#include <stdio.h> #include <windows.h>
int main() { RECT rc; SetRect(&rc, 100, 100, 200, 200); ClipCursor(&rc); getchar(); ClipCursor(NULL); return 0; }
If you're running Windows on a virtual machine, remember to disable things like "mouse pointer integration" before testing.
I don't run Windows here. And I cannot build programs on my office machine. If you want to compile the program and provide it via PM then feel free to do so. Otherwise, I will not be able to test this.
If you are in a virtual desktop, the mouse should stop at the edge of the window and move to where you re-enter it.
I think we all agree about that, but GetCursorPos currently doesn't.
That is brokenness then. Please continue to try and fix it. James McKenzie
2010/1/22 Lauri Kenttä <lauri.kentta(a)gmail.com>:
hi lauri. i'm glad you take people's comments with a grain of salt. vitaliy was a bit condescending but he said "please" :D it's unfortunate but we have to get used to it. i know it's not the best to entice developers but it's what we have, and it's good, stay around and you'll see ;) as to your issue, you should write tests for all that you can, that show bugs in wine and inconformances with windows. test driven development is appreciated. that little test app you shown is a start. do that first with todo_wine() where needed and you will have an easier walk. as to the clipping to virtual desktop problem, we are still waiting on someone to integrate xinput2 in wine. it should help alot for this kind of situation, there are alot of games that require it. see: http://bugs.winehq.org/show_bug.cgi?id=6971 that bug is why vitaliy is a bit more picky with this kind of patch ;) regards.
Ricardo Filipe wrote:
Ok, that really explains something. Thanks a lot, I'll find a game to test. -- Lauri Kenttä
On 01/22/2010 03:42 PM, Lauri Kenttä wrote: >> The SetCursorPos() calls hooks on Wine because Wine has big issue - it >> doesn't filter pointer move messages that were caused by Wine itself. > > The first patch (http://source.winehq.org/patches/data/57569) tries to > address exactly this. I think XWarpCursor is not a problem, as the event > comes asynchronously with some delay. So my patch fixes the easier but > worse case, calling hooks directly from SetCursorPos. > > What's wrong with this, why is it wrong, and do you have a better idea? > What else causes Wine to produce extra messages? I'm fine with this patch. According to your test SetCursorPos() indeed doesn't generate ll hook. Was an oversight on my part when I added that extra queue_raw_mouse_message() call. Just Keep the formatting the same (curly brace on it's own line). And remove FIXME comment - it's not the only place that calls XWarpPointer(). Oh and make it a series with your test patch. This patch being #1 and the test #2. > Also, is there a reason why I shouldn't write a test for this to make the > issue better known? By all means. The way things work in Wine - if tests fail on Wine you should use "todo_wine" at front of each failing ok() call. So 'make test' always succeeds. >>> I'm aware that my version causes hooks to be called with clipped >>> coordinates on mouse move, when they should in that case be called with >>> the unclipped ones. >> This particular change will break all games using dinput when mouse >> pointer goes outside of virtual desktop. > > (This was the elaboration I was asking for.) I didn't know that, and I > don't have any such games. Feel free to reject the patch, I'll rewrite it > without this bug and in smaller chunks. (It would be helpful if the first > two patches were also either accepted or rejected.) I'm not the one accepting or rejecting patches - Alexandre is. I'm just one of the developers who made lots of changes to that particular area of the code to match what native does. In this case I'm worrying about mouse movements on the edge of the screen. Remember that ll hook is supposed to be called with whatever came straight from the mouse driver. Not the pointer. And to test it you'll need to use injected events (mouse_event). SetCursorPos() won't do for this, it only alters pointer position. >>> returns unclipped coordinates from GetCursorPos >> This is what native will do inside hook handler. Add some tests. > That's not true. (See below.) Yes, you right here. I was thinking about something else - GetCursorPos() returns previous position while inside hook handler. > I've attached a program that outputs coordinates from hookproc and > wndproc, both the passed ones and the ones from GetCursorPos. If you try > it, you can see that Wine has the following bugs: > - incorrect coordinates in some WM:s > - incorrect coordinates in some hook calls > - incorrect coordinates and missing WM:s after clipping > - extra hook calls on SetCursorPos, even if the pos doesn't change All these can be fixed. But will need Wine tests to verify and keep it from breaking in the future. And need one patch for each problem unless it's a same fix for all of them. If things break separate patch per issue will help identify the change during regression testing. > - extra WM_MOUSEMOVE after zero-length moves Careful with this. Some games do require it. That's why it's there with the comment (in SetCursorPos). Or is there another source of them? > - some other extra hook calls and WM:s due to XWarpPointer To fix this Wine have to keep track of all XWarpPointer calls and all generated X events so it can filter ones that came from warping pointer. > As you can see, the correct order would be: > - call hooks for moving, return old pos from GetCursorPos > - clip and store the new position > - send the move message using the new position > - call hooks and send the button message, both using the clipped position Looks right. >> What exactly is wrong with [border scrolling]? > Many games (for example, Baldur's Gate) check for mouse coordinates. If > the pointer is outside the virtual desktop, Wine gives negative > coordinates and the game doesn't scroll left/up, because x != 0 and y != > 0. The same thing happens to right/bottom, of course. Correct clipping > fixes it. That explains why some games have this issue. >> Who said that Wine should keep the pointer inside virtual desktop? > Ok, I'll not do it. But don't you think it's a bit confusing if the > pointer moves freely but the program thinks it's clipped to an area? I've had that discussion with Alexandre. His point was - don't confine pointer to the virtual desktop because Wine has it exactly for the reason of letting you run full-screen games in the window, as a work around for games not having windowed mode. And to let you use your system when Wine crashes/locks up. The question of what to do when cursor is outside of VD - I personally thing we should just clip coordinates to VD and pass them to the app. Might need to do something different when Wine looses focus... > My apologies for bringing up something that is actually none of my > business, but I think you should pay more attention to the way you write > your comments. I'm sorry if some of my comments came extra harsh. Didn't intended that way. All I wanted to do is to review your patches and make sure you are aware of all the hidden gotchas in that piece of code. And of course help you get all your work past Alexandre. Vitaliy.
participants (4)
-
James McKenzie -
Lauri Kenttä -
Ricardo Filipe -
Vitaliy Margolen