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

Matteo Bruni matteo.mystral at gmail.com
Mon Sep 19 19:59:04 CDT 2016


2016-09-20 2:11 GMT+02:00 Fabian Maurer <dark.shadow4 at 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.



More information about the wine-devel mailing list