Re: [PATCH 1/5] d3dx11: Copied functions and defines from d3dx_36 related to texture loading
On 13.09.2016 23:11, Fabian Maurer wrote:
+/* temporary defines to not break compilation */ + +#define IDirect3DSurface9_GetDesc(p,a) (p)->lpVtbl->GetDesc(p,a) +#define IDirect3DSurface9_LockRect(p,a,b,c) (p)->lpVtbl->LockRect(p,a,b,c) +#define IDirect3DSurface9_UnlockRect(p) (p)->lpVtbl->UnlockRect(p) + ... +#define INTERFACE IDirect3DSurface9 +DECLARE_INTERFACE_(IDirect3DSurface9,IDirect3DResource9)
I don't think this is acceptable. It should be done in a way that doesn't introduce mess like this.
I don't think this is acceptable. It should be done in a way that doesn't introduce mess like this.
I split the patches but I didn't know how to do it . For not breaking compilation, I added these temporary defines. The later patch removes those again.
2016-09-13 22:28 GMT+02:00 Fabian Maurer <dark.shadow4(a)web.de>:
I don't think this is acceptable. It should be done in a way that doesn't introduce mess like this.
I split the patches but I didn't know how to do it . For not breaking compilation, I added these temporary defines. The later patch removes those again.
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. 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). 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, 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).
2016-09-20 2:11 GMT+02:00 Fabian Maurer <dark.shadow4(a)web.de>:
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.
Well, obviously a single huge patch wouldn't be okay either. The problem is in how you split stuff over multiple patches. More below...
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.
You aren't actually stripping functionality from anything. You are implementing a new d3dx11 function. The fact that it happens to be very similar to another function which is already implemented is quite tangential. I'd say it's been more confusing than helping up to this point. Try to go implement the d3dx11 functions as I suggest above. You should "automatically" get a much nicer patch series, with each patch adding some small and easily testable piece of functionality or a test for the aforementioned functionality. I'd start with a stub for D3DX11GetImageInfoFromMemory(). Then one or more patches adding flesh to it AND tests. Sharing code should not be a concern at all at this point. Looking at the d3dx9 code and tests for inspiration is okay but you DON'T want to copy it, that's simply going to be worse than starting from scratch.
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).
Don't worry about sharing code for the time being. Tests are much more important. WRT the tests there is the testbot (http://testbot.winehq.org/), there you can submit patches and run tests against a number of Windows VMs. You just have to ask for an account first. A local Windows VM should also be fine to test for these d3dx11 functions. In theory you could even use native d3dx11 on Wine, although that might give some misleading results e.g. if Wine's d3d11 isn't quite doing the right thing.
On 20 September 2016 at 02:59, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
functions. In theory you could even use native d3dx11 on Wine, although that might give some misleading results e.g. if Wine's d3d11 isn't quite doing the right thing.
And you'd have to be careful not to look at e.g. +d3d11 traces when doing that, so I'd recommend against it.
participants (4)
-
Fabian Maurer -
Henri Verbeet -
Matteo Bruni -
Nikolay Sivov