September 2016 - Wine-Devel

Re: [PATCH 2/5] d3dx11: Made texture loader use wined3dformat instead of d3dformat.

by Fabian Maurer

> It was already mentioned but, at the cost of being annoying, let me > reiterate: d3dx* DLLs can't directly use wined3d data types or APIs. > They have to go through the respective d3d* client library functions. > > I suspect that code sharing between d3dx* versions will eventually > happen in a somewhat different fashion and trying to build the "final > solution" from the ground up is going to be hard. Porting the various > pieces of code to d3d[x]11 as necessary, even at the cost of > "duplication" in the short term, is probably a better plan. I somewhat doubt it. It wasn't too hard to use the d3dx9 parts for d3dx11, and it shouldn't be too hard to make it a good generic version. It would make both d3dx9 and d3dx11 simpler, while providing modular abstraction. Just loading a texture or texture information is something that can be standalone. I'm open to change of course, but I'd like to understand the reason behind it. It's not linked to wined3d anymore, only uses the intermediate imageformat. Since a generic imageloader can't rely on DX9 types nor on DX11 and since I didn't want to reinvent the wheel, I just used the implemenation at hand: wined3dformat.

Sept. 20, 2016

Re: [PATCH 2/5] d3dx11: Made texture loader use wined3dformat instead of d3dformat.

by Matteo Bruni

2016-09-13 22:11 GMT+02:00 Fabian Maurer <dark.shadow4(a)web.de>: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> It was already mentioned but, at the cost of being annoying, let me reiterate: d3dx* DLLs can't directly use wined3d data types or APIs. They have to go through the respective d3d* client library functions. I suspect that code sharing between d3dx* versions will eventually happen in a somewhat different fashion and trying to build the "final solution" from the ground up is going to be hard. Porting the various pieces of code to d3d[x]11 as necessary, even at the cost of "duplication" in the short term, is probably a better plan.

Sept. 20, 2016

Re: [PATCH 1/2] dinput: Fix phase conversion on periodic effects

by Bruno Jesus

On Mon, Sep 19, 2016 at 9:11 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> I have been talking a lot to Elias Vanderstuyft recently, we are aware >> of precision problems and are working to fix it a in better way. For >> now I'm sticking the simple changes that I'm able to test in my real >> hardware. The limit of phase is 35999 because it is related to degrees >> / 100, more than that would wrap. You are probably right that we need >> to clamp to max value before calculating. > > I don't know if we need to clamp or not. My guess is that it just > repeats periodically after that, if such values are allowed at all > (SetParameter might fail). I.e. I would have used something like > (phase % 36000) instead of clamping. > That probably means some more tests are required... > >> It is important to note that >> although we are losing precision you can't really notice it in a >> standard gamepad. > > Sure but it still seems like a good idea to do things right when reasonable. > Notice that I don't mean to blame you or the patch, I had a bit of a > look at the existing code after all... Yes, I agree that if we can make it better it should be better. I just lack the math skills sometimes for things that may look obvious for other people, I even considered opening questions at http://math.stackexchange.com to help improve some equations.

Sept. 20, 2016

Re: [PATCH 1/5] d3dx11: Copied functions and defines from d3dx_36 related to texture loading

by Fabian Maurer

> Yeah, that's certainly not acceptable. Also in general introducing > dead code (i.e. code that's unused until a later patch) is not okay. Well I see. It's just that I didn't want to commit the whole thing as single patch, since that would make it way less understandable. So I split it up to show where exactly I changed lines. Regarding "introducing dead code", I thought it was fine if it was part of a patchset? Obviously all patches in this set depend on the others. They aren't supposed to be on their own, it's just way more overseeable like that. > You can probably start from a simple implementation of > D3DX11CreateTextureFromMemory(), without any scaling or format > conversion and without DDS support. Something like > 258dba1a521cff3226db523b012ef3de2599e578 but for d3dx11. Notice how > that calls D3DXGetImageInfoFromFileInMemory(); similarly it might make > sense to implement D3DX11GetImageInfoFromMemory() first. > > Then you should introduce the various other pieces incrementally, as > they become necessary. You don't want to actually duplicate the code > keeping all the d3d9-isms, you'll have to rewrite those helpers in a > way that makes sense for d3d11 (incidentally, that's what I meant with > this being a useful step before figuring out how generic helpers would > look like). Actually, this is my attempt at a generic helper. "load_imagedata_from_file_in_memory" and "load_imageinfo_from_file_in_memory" should be usable for both DX9 and DX11. The point is, that this is mostly C&P from d3dx9 code. I didn't change too much, so I figured it would be a bad idea to cut out functionality to add it later again. It's about extracting the functionality from d3dx9 into something that's reusable. I could have extracted and used it for d3dx9 first, but we don't know yet where to put it.. > Most importantly, you need to write some tests for all the functions > you are implementing. The related d3dx9 tests are probably a decent > starting point, although there are a few new bits that should be > tested as well (e.g. it would be interesting to test various > combinations of BindFlags, CpuAccessFlags and MiscFlags). You want to > do that even before starting to work on the implementation, there > might be some surprising results. Yeah, I don't have tests yet. First I wanted to give you an implementation so we could decide on where to put the helpers. Also, I kinda don't know how to write a proper test without running windows (except for a VM).

Sept. 20, 2016

Re: [PATCH 1/2] dinput: Fix phase conversion on periodic effects

by Matteo Bruni

2016-09-20 1:34 GMT+02:00 Bruno Jesus <00cpxxx(a)gmail.com>: > On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> 2016-09-14 7:25 GMT+02:00 Bruno Jesus <00cpxxx(a)gmail.com>: >>> Based on idea by Elias Vanderstuyft. >>> >>> Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com> >>> --- >>> dlls/dinput/effect_linuxinput.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> Thanks for giving dinput and force feedback some love and care lately. >> I'm going to be a nag though... > > Ciao, Italian friend. ;) >>> diff --git a/dlls/dinput/effect_linuxinput.c b/dlls/dinput/effect_linuxinput.c >>> index 9205c9c..bb45792 100644 >>> --- a/dlls/dinput/effect_linuxinput.c >>> +++ b/dlls/dinput/effect_linuxinput.c >>> @@ -632,7 +632,8 @@ static HRESULT WINAPI LinuxInputEffectImpl_SetParameters( >>> >>> This->effect.u.periodic.magnitude = (tsp->dwMagnitude / 10) * 32; >>> This->effect.u.periodic.offset = (tsp->lOffset / 10) * 32; >>> - This->effect.u.periodic.phase = (tsp->dwPhase / 9) * 8; /* == (/ 36 * 32) */ >>> + /* phase ranges from 0 - 35999 in dinput and 0 - 65535 on linux */ >>> + This->effect.u.periodic.phase = (tsp->dwPhase / 36) * 65; >> >> Probably I'm missing something but... Why don't you actually use 35999 >> and 65535 in the expression? >> Relatedly, why not do the multiplication before the division instead >> of throwing bits of precision away? Maybe phase can go over 35999? If >> that's the only reason, I'd adjust the phase before doing this >> computation instead. >> I guess these points apply to the other effect parameters too. > > I have been talking a lot to Elias Vanderstuyft recently, we are aware > of precision problems and are working to fix it a in better way. For > now I'm sticking the simple changes that I'm able to test in my real > hardware. The limit of phase is 35999 because it is related to degrees > / 100, more than that would wrap. You are probably right that we need > to clamp to max value before calculating. I don't know if we need to clamp or not. My guess is that it just repeats periodically after that, if such values are allowed at all (SetParameter might fail). I.e. I would have used something like (phase % 36000) instead of clamping. That probably means some more tests are required... > It is important to note that > although we are losing precision you can't really notice it in a > standard gamepad. Sure but it still seems like a good idea to do things right when reasonable. Notice that I don't mean to blame you or the patch, I had a bit of a look at the existing code after all... >> BTW, you didn't change the corresponding piece of code in >> GetParameters (which, for some reason, uses 33 and 36 as scaling >> constants...) so that's probably broken now. Or at least more broken >> than it was before. > > I'm planning to throw away the GetParameters code and instead save a > copy of SetParameters value and return it instead. Yeah, that sounds like a good idea for sure.

Sept. 20, 2016

Re: [PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

by Matteo Bruni

2016-09-20 1:56 GMT+02:00 Fabian Maurer <dark.shadow4(a)web.de>: > Thanks for your feedback. > >> > + UINT filename_converted_len; >> >> Probably you can call it just "size". > Are you sure? I tried to make it less ambiguous, since there are two sizes > used. Right. Whatever is fine I guess, but using "_size" instead of "_len" seems valuable for consistency. >> This would be okay otherwise but, as mentioned for patch [4/5], maybe >> it's a good idea to implement and make use of >> D3DX11CreateAsyncFileLoaderW() here too. > > I fixed the other issues on my local branch and I'll try to get the async parts > right. Cool, looking forward to it.

Sept. 20, 2016

Re: [PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

by Fabian Maurer

Thanks for your feedback. > > + UINT filename_converted_len; > > Probably you can call it just "size". Are you sure? I tried to make it less ambiguous, since there are two sizes used. > This would be okay otherwise but, as mentioned for patch [4/5], maybe > it's a good idea to implement and make use of > D3DX11CreateAsyncFileLoaderW() here too. I fixed the other issues on my local branch and I'll try to get the async parts right.

Sept. 19, 2016

Re: [PATCH 5/5] d3dx11: Implemented D3DX11CompileFromFileW

by Matteo Bruni

2016-09-13 22:11 GMT+02:00 Fabian Maurer <dark.shadow4(a)web.de>: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/d3dx11_43/async.c | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) > > diff --git a/dlls/d3dx11_43/async.c b/dlls/d3dx11_43/async.c > index 90c60fa..1e3042b 100644 > --- a/dlls/d3dx11_43/async.c > +++ b/dlls/d3dx11_43/async.c > @@ -1989,16 +1989,36 @@ HRESULT WINAPI D3DX11CompileFromFileA(const char *filename, const D3D10_SHADER_M > return E_NOTIMPL; > } > > + Stray newline. > HRESULT WINAPI D3DX11CompileFromFileW(const WCHAR *filename, const D3D10_SHADER_MACRO *defines, > ID3D10Include *include, const char *entry_point, const char *target, UINT sflags, UINT eflags, > ID3DX11ThreadPump *pump, ID3D10Blob **shader, ID3D10Blob **error_messages, HRESULT *hresult) > { > - FIXME("filename %s, defines %p, include %p, entry_point %s, target %s, sflags %#x, " > + void *file_buffer; > + HRESULT hr; > + DWORD file_size; > + LPSTR filename_converted; Please use "char *" instead. > + UINT filename_converted_len; Probably you can call it just "size". > + > + TRACE("filename %s, defines %p, include %p, entry_point %s, target %s, sflags %#x, " > "eflags %#x, pump %p, shader %p, error_messages %p, hresult %p stub.\n", The "stub" part should go away. > debugstr_w(filename), defines, include, debugstr_a(entry_point), debugstr_a(target), > sflags, eflags, pump, shader, error_messages, hresult); > > - return E_NOTIMPL; > + if (FAILED(hr = map_view_of_file(filename, &file_buffer, &file_size))) > + return hr; > + > + filename_converted_len = WideCharToMultiByte(CP_ACP, 0, filename, -1, NULL, 0, NULL, NULL); > + filename_converted = HeapAlloc(GetProcessHeap(), 0, filename_converted_len); > + WideCharToMultiByte(CP_ACP, 0, filename, -1, filename_converted, filename_converted_len, NULL, NULL); > + > + hr = D3DX11CompileFromMemory(file_buffer, file_size, filename_converted, defines, include, entry_point, target, > + sflags, eflags, pump, shader, error_messages, hresult); > + > + HeapFree(GetProcessHeap(), 0, filename_converted); > + UnmapViewOfFile(file_buffer); > + > + return hr; > } This would be okay otherwise but, as mentioned for patch [4/5], maybe it's a good idea to implement and make use of D3DX11CreateAsyncFileLoaderW() here too.

Sept. 19, 2016

Re: [PATCH 1/2] dinput: Fix phase conversion on periodic effects

by Bruno Jesus

On Mon, Sep 19, 2016 at 8:12 PM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > 2016-09-14 7:25 GMT+02:00 Bruno Jesus <00cpxxx(a)gmail.com>: >> Based on idea by Elias Vanderstuyft. >> >> Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com> >> --- >> dlls/dinput/effect_linuxinput.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > Thanks for giving dinput and force feedback some love and care lately. > I'm going to be a nag though... Ciao, Italian friend. >> diff --git a/dlls/dinput/effect_linuxinput.c b/dlls/dinput/effect_linuxinput.c >> index 9205c9c..bb45792 100644 >> --- a/dlls/dinput/effect_linuxinput.c >> +++ b/dlls/dinput/effect_linuxinput.c >> @@ -632,7 +632,8 @@ static HRESULT WINAPI LinuxInputEffectImpl_SetParameters( >> >> This->effect.u.periodic.magnitude = (tsp->dwMagnitude / 10) * 32; >> This->effect.u.periodic.offset = (tsp->lOffset / 10) * 32; >> - This->effect.u.periodic.phase = (tsp->dwPhase / 9) * 8; /* == (/ 36 * 32) */ >> + /* phase ranges from 0 - 35999 in dinput and 0 - 65535 on linux */ >> + This->effect.u.periodic.phase = (tsp->dwPhase / 36) * 65; > > Probably I'm missing something but... Why don't you actually use 35999 > and 65535 in the expression? > Relatedly, why not do the multiplication before the division instead > of throwing bits of precision away? Maybe phase can go over 35999? If > that's the only reason, I'd adjust the phase before doing this > computation instead. > I guess these points apply to the other effect parameters too. I have been talking a lot to Elias Vanderstuyft recently, we are aware of precision problems and are working to fix it a in better way. For now I'm sticking the simple changes that I'm able to test in my real hardware. The limit of phase is 35999 because it is related to degrees / 100, more than that would wrap. You are probably right that we need to clamp to max value before calculating. It is important to note that although we are losing precision you can't really notice it in a standard gamepad. > BTW, you didn't change the corresponding piece of code in > GetParameters (which, for some reason, uses 33 and 36 as scaling > constants...) so that's probably broken now. Or at least more broken > than it was before. I'm planning to throw away the GetParameters code and instead save a copy of SetParameters value and return it instead. This will solve the issues about linux -> dinput reconversion, this seems to work but testing takes longer time. Specially because I could not find any game that actually uses this, only effect editing tools. Best wishes, Bruno

Sept. 19, 2016

Re: [PATCH 4/5] d3dx11: Implemented D3DX11CreateShaderResourceViewFromFileW

by Matteo Bruni

2016-09-13 22:27 GMT+02:00 Nikolay Sivov <bunglehead(a)gmail.com>: > On 13.09.2016 23:11, Fabian Maurer wrote: >> +HRESULT WINAPI D3DX11CreateShaderResourceViewFromFileW(ID3D11Device *iface, >> + WCHAR *filename, D3DX11_IMAGE_LOAD_INFO *load_info_original, ID3DX11ThreadPump *pump, ID3D11ShaderResourceView **view, HRESULT *hresult) >> +{ >> + D3DX11_IMAGE_LOAD_INFO load_info; >> + D3D11_SHADER_RESOURCE_VIEW_DESC desc_view; >> + D3D11_TEXTURE2D_DESC desc_texture; >> + D3D11_SUBRESOURCE_DATA data_texture; >> + ID3D11Texture2D *texture; >> + enum wined3d_format_id dst_format; >> + UINT dst_pitch; >> + D3DXIMAGE_INFO image_info; >> + void *file_buffer = NULL; >> + DWORD file_size; >> + void *image_data = NULL; >> + RECT rectSize; >> + HRESULT hr; >> + >> + TRACE("iface %p, filename %s, load_info_original %p, pump %p, view %p, hresult %p.\n", >> + iface, debugstr_w(filename), load_info_original, pump, view, hresult); >> + >> + if (pump) >> + FIXME("Unimplemented ID3DX11ThreadPump handling.\n"); > > Personally I think this function should use > D3DX11CreateAsyncFileLoaderW() and > D3DX11CreateAsyncShaderResourceViewProcessor() internally, for both > async and synchronous cases. This will make support for a thread pump > trivial, and will consolidate things like map_view_of_file() in one place. Yeah, it would be nice. The async and sync cases should be able to share most of the code, with the only difference being that in the async case the function will call AddWorkItem on the thread pump object while in the sync case it will call the data loader / data processor methods itself. While it might be an option to ignore the thread pump case in the initial implementation, it seems clearly better to structure things to use data loader + data processor objects from the start.

Sept. 19, 2016