Re: Re: [WineD3D] fix bug 4872

----Message d'origine----
> Date: Wed, 14 Jun 2006 22:03:58 -0600
> De: Vitaliy Margolen 
> A: Raphael 
> Copie à: [e-mail]
> Sujet: Re: [WineD3D] fix bug 4872
>
> Wednesday, June 14, 2006, 6:51:21 PM, Raphael wrote:
> > On Thursday 15 June 2006 02:34, Raphael wrote:
> > > Hi,
> > >
> > > first patch since a long time :)
> > >
> > > Changelog:
> > > - rename WineD3D_(Create|Release)FakeGLContext to

WineD3D_(Create|
> > > Release)FakeGLContextOnNeed
> > > - and if a GL context is already active (and was not a fake

context)
> > > returns simili fake context with current GL context infos
> > > - cleanup not more needed checks
> > >
> > > fix #4872: make IWineD3DImpl_CheckDeviceType working if a GL

context is
> > > already active
> > >
> > > Regards,
> > > Raphael
> > Use this patch instead, i let some stupid (and buggy) debug code
> > Regards,
> > Raphael
>
> I think I counted at least 5 indentation stiles in your patch. I

understand
> staying consistent with the code's style and using your own stile. But

5?!

No i only see two problems, and made by use of xemacs
(as i have broken my kate install). Anyway i plan to cleanup
code in file in a next patch

See 
http://marc.theaimsgroup.com/?l=wine-patches&m=115033272215006&w=2
> > }
>
> You forgot LEAVE_GL();

NO, LEAVE_GL() is done on ReleaseFakeGLContextOnNeed.
to permit between two calls to can make X/GLX calls safely
> > - case WINED3DFMT_A4R4G4B4:
> > - if (4 == rb && 4 == gb && 4 == bb && 4 == ab) return TRUE;
> > - break;
>
> Has nothing to do with context.

??
> > - case WINED3DFMT_A8:
> > - if (8 == ab) return TRUE;
> > - break;
>
> Same here

??
> >  if (Adapter >= IWineD3D_GetAdapterCount(iface)) {
> > -
> >    TRACE("(%p) Failed: Atapter (%u) higher than supported 

adapters (%u) returning WINED3DERR_INVALIDCALL\n", This, 
Adapter, IWineD3D_GetAdapterCount(iface));
> > -
> > TRACE_(d3d_caps)("[FAILED: Atapter (%u) higher than 

supported adapters (%u)]\n", Adapter, 
IWineD3D_GetAdapterCount(iface));
> >      return WINED3DERR_INVALIDCALL;
> >  }
>
> This one as well. And I haven't seen any traces so far that used

square
> brackets. Now we will!

Have you ever seen D3D traces ?

I only made same as IWineD3DImpl_CheckDeviceFormat
(for coherency)

see http://source.winehq.org/source/dlls/wined3d/directx.c#L1425

Anyway displaying [OK] or [FAILED] for checks results seems a good 
idea (and easy to find in logs)
> Vitaliy Margolen

Regards,
Raphael

PS: before many complains, my ISP webmail is buggy, sorry :)

[WineD3D] fix bug 4872

[Ivan Gyurdiev]

> No i only see two problems, and made by use of xemacs

(as i have broken my kate install).

If vi was used this never would have happened :)
> PS: before many complains, my ISP webmail is buggy, sorry :)

If POP3 was used, this never would have happened :)

Seriously though - it's a major pain to read those emails, at least in 
Thunderbird - you have to download the attachment (since it isn't 
recognized as text), and open it up in an editor. Is there no way to fix 
this? I like to read all the d3d discussions, but it's a hassle...
> Have you ever seen D3D traces ?
> see http://source.winehq.org/source/dlls/wined3d/directx.c#L1425

Not that I have an opinion on the subject, but it might be useful to 
notice that this is the only function in the wined3d codebase that uses 
them [ according to grep at least ].

[WineD3D] fix bug 4872

[Mike McCormack]

Ivan Gyurdiev wrote:
> Seriously though - it's a major pain to read those emails, at least in 
> Thunderbird - you have to download the attachment (since it isn't 
> recognized as text), and open it up in an editor. Is there no way to fix 
> this? I like to read all the d3d discussions, but it's a hassle...

I simply don't read them, and I guess alot of other people don't either.

X-Mailer: Medianet/v2.0
  Content-Type: undefined

It seems like there's been a few different complaints about 
Medianet/v2.0, so I suggest you use an email client that sets the 
content type correctly to post to wine-devel in future.

Mike

[WineD3D] fix bug 4872

[Vitaliy Margolen]

Thursday, June 15, 2006, 1:23:37 AM, Raphael wrote:
> > Wednesday, June 14, 2006, 6:51:21 PM, Raphael wrote:
> > > On Thursday 15 June 2006 02:34, Raphael wrote:
> > > > Hi,
> > > >
> > > > first patch since a long time :)
> > > >
> > > > Changelog:
> > > > - rename WineD3D_(Create|Release)FakeGLContext to WineD3D_(Create|
> > > >
> > > > Release)FakeGLContextOnNeed
> > > > - and if a GL context is already active (and was not a fake context)
> > > >
> > > > returns simili fake context with current GL context infos
> > > > - cleanup not more needed checks
> > > >
> > > > fix #4872: make IWineD3DImpl_CheckDeviceType working if a GL context is
> > > > already active
> > > Use this patch instead, i let some stupid (and buggy) debug code
> >
> > I think I counted at least 5 indentation stiles in your patch. I understand
> > staying consistent with the code's style and using your own stile. But 5?!
> No i only see two problems, and made by use of xemacs
> (as i have broken my kate install). Anyway i plan to cleanup
> code in file in a next patch

No you have:
1. 2 spaces
2. 3 spaces
3. 4 spaces
4. spaces no tabs
5. spaces with tabs

And just formatting change patches are not accepted. So you better have it right
the first rime.
> > > }
> >
> > You forgot LEAVE_GL();
> NO, LEAVE_GL() is done on ReleaseFakeGLContextOnNeed.
> to permit between two calls to can make X/GLX calls safely

So where are you disposing of it then? I can't find the place that will delete
this context on device delete.
> > > - case WINED3DFMT_A4R4G4B4:
> > > - if (4 == rb && 4 == gb && 4 == bb && 4 == ab) return TRUE;
> > > - break;
> >
> > Has nothing to do with context.
>
> ??
> > > - case WINED3DFMT_A8:
> > > - if (8 == ab) return TRUE;
> > > - break;
> >
> > Same here
>
> ??

These changes has nothing to do with what stated in the changelog. Please make a
separate patch for them.
> > >  if (Adapter >= IWineD3D_GetAdapterCount(iface)) {
> > > -
> > >    TRACE("(%p) Failed: Atapter (%u) higher than supported
> > >
> > > adapters (%u) returning WINED3DERR_INVALIDCALL\n", This,
> > > Adapter, IWineD3D_GetAdapterCount(iface));
> > > -
> > > TRACE_(d3d_caps)("[FAILED: Atapter (%u) higher than 
> > >
> > > supported adapters (%u)]\n", Adapter,
> > > IWineD3D_GetAdapterCount(iface));
> > >          return WINED3DERR_INVALIDCALL;
> > >      }
> >
> > This one as well. And I haven't seen any traces so far that used square
> > brackets. Now we will!
> Have you ever seen D3D traces ?
> I only made same as IWineD3DImpl_CheckDeviceFormat
> (for coherency)
> Anyway displaying [OK] or [FAILED] for checks results seems a good 
> idea (and easy to find in logs)

Please don't add this sort of stuff. You are correct as-is d3d traces look horrid
and impossible to find anything. Adding one more type of "screaming at you"
output will not make it more readable. It's only used in one place, so I don't
see why would you accent this error out of any other related stuff?

Vitaliy

[WineD3D] fix bug 4872

[Raphael]

> > > > }
> > >
> > > You forgot LEAVE_GL();
> >
> > NO, LEAVE_GL() is done on ReleaseFakeGLContextOnNeed.
> > to permit between two calls to can make X/GLX calls safely
>
> So where are you disposing of it then? I can't find the place that will
> delete this context on device delete.

what device delete ?
ReleaseFakeGLContextOnNeed only delete GLXContext if it was a temporary GLX 
Context (ie. created by CreateFakeGLContextOnNeed, if no one currently 
available)
> Please don't add this sort of stuff. You are correct as-is d3d traces look
> horrid and impossible to find anything.

Depends if you know what you search ... :)
Anyway, i agree that we must do more work on d3d traces
> Adding one more type of "screaming 
> at you" output will not make it more readable. It's only used in one place,
> so I don't see why would you accent this error out of any other related
> stuff?

Simply because it's the main cause why too many games complains about D3D init 
failure.
And having [OK] [FAILED] is more readable than TRACE("returns %d\n", hr)
> Vitaliy

Regards,
Raphael