Re: winex11.drv: Added missing mouse position clipping. (try 2)
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 coordina
Re: winex11.drv: Added missing mouse position clipping. (try 2)
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
> #include
>
> 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
Re: winex11.drv: Added missing mouse position clipping. (try 2)
Ricardo Filipe wrote: > see: http://bugs.winehq.org/show_bug.cgi?id=6971 Ok, that really explains something. Thanks a lot, I'll find a game to test. -- Lauri Kenttä
Re: winex11.drv: Added missing mouse position clipping. (try 2)
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
#include
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ä
Re: winex11.drv: Added missing mouse position clipping. (try 2)
2010/1/22 Lauri Kenttä : > 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.
Re: winex11.drv: Added missing mouse position clipping. (try 2)
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
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ä
#define _WIN32_WINNT 0x0500
#include
#include
int stage = 0;
RECT *tmp_rect(int x0, int y0, int x1, int y1) {
static RECT r;
SetRect(&r, x0, y0, x1, y1);
return &r;
}
INPUT *tmp_input(DWORD flags, int x, int y) {
static INPUT input;
input.type = INPUT_MOUSE;
input.mi.dwFlags = flags;
input.mi.dx = x;
input.mi.dy = y;
return &input;
}
const char *tmp_pos_s
Re: winex11.drv: Added missing mouse position clipping. (try 2)
Lauri Kenttä wrote: > The new series of patches has some related changes (including a new test) > applied before this one. You have some issues in your test as well. The SetCursorPos() calls hooks on Wine because Wine has big issue - it doesn't filter pointer move messages that were caused by Wine itself. What you did in your patch is a half fix the wrong way. > Could you please elaborate that "so many"? I can't give you all the programs/games that depend on particular behavior - there are big number of them. > 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. > returns unclipped coordinates from GetCursorPos This is what native will do inside hook handler. Add some tests. > and even sends incorrect WM_* messages. Tests please. > This is clearly wrong and causes many problems (example: border > scrolling is next to impossible in many full-screen games when played on > Wine virtual desktop). What exactly is wrong with them? And what part are you trying to fix? > I sincerely think my version is less likely to break anything. Surely > there are more applications using WM_* and GetCursorPos than hooks. This is not a reason to break anything. You either fix things without breaking anything else. Or you don't touch the code at all. > Anyone could do exactly the same thing "manually" by calling SetCursorPos, > so why shouldn't ClipCursor do it automatically? Also, apparently Wine > won't (or can't) warp the mouse if it lies outside the virtual desktop. Number of reasons: - Who said that Wine should keep the pointer inside virtual desktop? It's there for the exact reason so users can have other apps running outside of Wine at the same time as full-screen game inside Wine. - You doing it wrong. Warping pointer is not the right way. If you really must do it, then you should be using pointer grab. Also you are not dealing with all the extra pointer move messages generated as a result of XWarpPointer(). Vitaliy.
Re: winex11.drv: Added missing mouse position clipping. (try 2)
Thanks for your comments, Vitaliy. At first, I must point out that you're commenting on a patch that has already been superseded. Not that there's much difference, but the new series of patches has some related changes (including a new test) applied before this one. Vitaliy Margolen wrote: > Lots of those areas you touch will break oh so many apps. Could you please elaborate that "so many"? 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. Still, please note that the current version calls all hooks (not just on mouse move) with the unclipped coordinates, returns unclipped coordinates from GetCursorPos and even sends incorrect WM_* messages. This is clearly wrong and causes many problems (example: border scrolling is next to impossible in many full-screen games when played on Wine virtual desktop). I sincerely think my version is less likely to break anything. Surely there are more applications using WM_* and GetCursorPos than hooks. Maybe I will have time to fix the remaining bugs some day. I will also try to write some Wine tests so that these problems (be they in my version or the old one) are not forgotten. Vitaliy Margolen wrote: > And don't try to keep pointer inside Wine's window Anyone could do exactly the same thing "manually" by calling SetCursorPos, so why shouldn't ClipCursor do it automatically? Also, apparently Wine won't (or can't) warp the mouse if it lies outside the virtual desktop. As I see it, clipping should first be implemented correctly, because that's what Windows applications expect. Then, if this causes problems, we could use a registry key similar to DXGrab to disable (or restrict) XWarpPointer usage. -- Lauri Kenttä
Re: winex11.drv: Added missing mouse position clipping. (try 2)
Lauri Kenttä wrote: > When the cursor position is clipped with ClipCursor, the cursor should be > confined to that area. This patch fixes cursor position clipping in > X11DRV_ClipCursor, X11DRV_GetCursorPos, X11DRV_SetCursorPos and > X11DRV_send_mouse_input and makes the mouse warp to the new position if > moved outside the clip rect. > > The previous try fixed only clipping (not warping) and only in > GetCursorPos, but the problem wasn't that small after all. > --- > dlls/winex11.drv/mouse.c | 52 > - > 1 files changed, 41 insertions(+), 11 deletions(-) > You changing way too many things without a single test. Lots of those areas you touch will break oh so many apps. For starters write some tests and verify what you doing is correct. You should be able to write tests for all injected messages (send_mouse_event). If you can't make an automated test, write a visual test for the rest and send to wine-devel. Especially pay attention to all messages sent, and order of calling hook, sending messages, modifying values returned from GetCursorPos(). You breaking that. And don't try to keep pointer inside Wine's window - this won't get past AJ. Vitaliy.
