Re: [PATCH 1/5] wined3d: Vertex fog uses the absolute eye position z (v2).

[Henri Verbeet]

On 26 November 2014 at 19:08, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
> +        conv.f = tests[i].start;
> +        hr = IDirect3DDevice8_SetRenderState(device, D3DRS_FOGSTART, conv.d);
> I don't care particularly much, but it might be nicer to just add the
> union to tests[], and then just use "tests[i].start.d" here.
> START_TEST(ddraw7)
>  {
>      HMODULE module = GetModuleHandleA("ddraw.dll");
> @@ -8089,4 +8190,5 @@ START_TEST(ddraw7)
>      test_resource_priority();
>      test_surface_desc_lock();
>      fog_interpolation_test();
> +    test_negative_fixedfunction_fog();
> I prefer this name, but I think it's more important that equivalent
> tests have the same name between D3D versions. I'd probably just
> rename the d3d8 and d3d9 versions instead. For what it's worth, I
> still plan to merge device.c and visual.c at some point in the future,
> and I'll probably fixup the naming of the existing tests in visual.c
> around that time as well.
> if (settings->ortho_fog)
> +            {
>                  /* Need to undo the [0.0 - 1.0] -> [-1.0 - 1.0] transformation from D3D to GL coordinates. */
>                  shader_addline(buffer, "gl_FogFragCoord = gl_Position.z * 0.5 + 0.5;\n");
> +            }
>              else
> +            {
>                  shader_addline(buffer, "gl_FogFragCoord = ec_pos.z;\n");
> +                if (!settings->transformed)
> +                    shader_addline(buffer, "gl_FogFragCoord = abs(gl_FogFragCoord);\n");
> +            }

I think this looks pretty messy. I'd suggest

    if (settings->ortho_fog)
       shader_addline(...);
    else if (!settings->transformed)
       shader_addline(...);
    else
       shader_addline(...);

[PATCH 1/5] wined3d: Vertex fog uses the absolute eye position z (v2).

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-11-27 10:22, schrieb Henri Verbeet:
> For what it's worth, I still plan to merge device.c and visual.c
> at some point in the future,
> I don't like this idea. I prefer to be able to run the visual tests on
> Windows without the mode setting tests in device.c messing up my
> screen and taking forever on my slow monitor. I'm also not a fan of
> 30k lines of code in a file, although that doesn't mean we have to go
> to the opposite extreme and add hundreds of 50-line .java files.

(Yeah, I can comment out unused tests while working on a new one, but
that's awkward too.)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUdvNMAAoJEN0/YqbEcdMwiH8P/01Nb8L4AvvJSedspW7DVrfN
Auotg4zhcjEaQ6XwqIWp4H/WkItJemHV871z3UQ7gLUSSbSa2+iONtd0NUpqbCJj
H7AFh+jzHI1M2TUDHTzaXekMQ/XW/mtfaDDSwsjqG56K2S47fbSjN8FkmyNx5T6J
BuU5lDz0w0dB/3L4VxGOd68Pqwag2DIcAzsmKUY3bVtWpuuY0dfZad7RJAJ95KIt
UvSVL/grqN4zH33VwRrkUkgWfYwAtuLgg5DjjzWlaFPETbwT0iIzMcD2BawCPOfy
KuoaP/ythS7zOwflijE+y46rCqLS70W66px56xHt3JKZcUFw5CAban2TNxFCGl1h
VGXQTDNV1U3YeC+v+lpBuRycIrGkoTncXEoyH0qSX8PV/AcWlcZIpSuDtLV27dir
qsX198zm311eFPfLo4OK5ATxvolBMjNS7gS0YEXPoJoH6hPQX2R+VeLXc3lRICaw
NKUu0zSLSvm4ysufSZvMGlzdrxTFMxIT5+tccNUN4cTvBvT9bjqBbpCkPrZlcAkv
oplV32aVOJ4RXaYAoKEYhmDEccIFlsLAYfHIqKpLp0wTdd+s2nI65P/U/K4E1Ut4
v65nUx7xixqJmpMiPi6bZgVP3hoElTK8Kvs1bFyylGzWIQWZNv3flRpRTWfY2PT1
ffnYZKN8qRo/kpOLz2n6
=raX1
-----END PGP SIGNATURE-----

[PATCH 1/5] wined3d: Vertex fog uses the absolute eye position z (v2).

[Henri Verbeet]

On 27 November 2014 at 10:47, Stefan Dösinger <stefandoesinger(a)gmail.com> wrote:
> > For what it's worth, I still plan to merge device.c and visual.c
> > at some point in the future,
> > I don't like this idea. I prefer to be able to run the visual tests on
> > Windows without the mode setting tests in device.c messing up my
> > screen and taking forever on my slow monitor. I'm also not a fan of
> > 30k lines of code in a file, although that doesn't mean we have to go
> > to the opposite extreme and add hundreds of 50-line .java files.

Actually, new tests should in general go into device.c although we
haven't really been enforcing that, in part because we currently don't
have the get_surface_color() / getPixelColor() helpers in device.c.
Merging would help with duplication of helper functions, and also be
more consistent with the ddraw, d3d10core and d2d1 tests. D3d9 in
particular is weird because it has the device/visual split for d3d9,
but a single file for d3d9ex.

Perhaps some of the mode setting tests can be merged together,
although obviously at the cost of increased complexity, or perhaps we
can e.g. introduce a WINETEST_ environment variable to avoid running
them. I think improving the mode setting tests is a separate issue
from merging device.c and visual.c though.

As an aside, many of the Present() calls in visual.c are really only
useful when debugging, and just removing them may speed things up as
well, although I haven't verified that.
> (Yeah, I can comment out unused tests while working on a new one, but
> that's awkward too.)
> I pretty much already always do that. In part because it helps to
> avoid depending on side effects of previous tests, and in part because
> it's just so much faster, especially on the testbot.

[PATCH 1/5] wined3d: Vertex fog uses the absolute eye position z (v2).

[Stefan Dösinger]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 2014-11-27 11:31, schrieb Henri Verbeet:
> Actually, new tests should in general go into device.c although we 
> haven't really been enforcing that, in part because we currently
> don't have the get_surface_color() / getPixelColor() helpers in
> device.c. Merging would help with duplication of helper functions,
> and also be more consistent with the ddraw, d3d10core and d2d1
> tests. D3d9 in particular is weird because it has the device/visual
> split for d3d9, but a single file for d3d9ex.
> Well, there's one point of separation: device.c and d3d9ex.c test
> device and resource creation, which is an area with differences
> between d3d9 and d3d9ex. Visual.c tests rendering once resources are
> created, and so far we haven't seen a difference here between d3d9 and
> d3d9ex.

Is there some overlap between device.c and visual.c? Sure. But I don't
think that means we should put everything into one huge file.

Ddraw is somewhat different because the differences between ddraw
versions are bigger and actually affect rendering.

(Note that this isn't something I'd wager a holy war about. I just
think fairly minor overlap between device.c and visual.c leads you to
the opposite extreme of Java's tiny files.)
> Perhaps some of the mode setting tests can be merged together, 
> although obviously at the cost of increased complexity,
> I'm pessimistic about that. I looked a bit into it (not much I must
> admit, just a bit), and I ran into problems pretty quickly, e.g. that
> the w8 ddraw driver crashes when calling GetSurfaceDesc after an
> external mode change. Reactivating a device on Windows in d3d8/9 is
> tricky, fvwm doesn't make this any easier on the other side, etc. I
> don't think we'll be able to cut down the number of mode changes
> substantially without sacrificing reliability.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJUeFnKAAoJEN0/YqbEcdMwiewP/RTdTPHXbEPutw5/o/KvgYnb
MV1iG3QzLIAN0Fg3lf/xzMhem3/izQtL5T7yZs+p3u8DSzCVoDkdTAzrsLiRn25r
HEur3zibds7UP/RluNvaPb/tS2tSac2M3+orHytdhh1kmiJfPsftRuCabCe0poU6
KfFMOFi7D9tyGHA1WJZiN6YbTZqX82pd9Dpbzk83ypLiOwjNfxufbPREzKICIgwR
85dG1vUX9tGdTzX7dn0wyyevD/NsSUdhXF9V6IthTJtdWpqlzVG287jm0gEIlw7L
gk6jvwcCzv+P/VpepUORO75MTgjELI9MgRsex4pXgqneL/ldHwMvkmn5iCSEdd4y
p9NaSzoSIXuFC7vKmBm1bkxHgov3/HcmspU14pIKpy3qKHAKHtyTRgbx3OLQqm5L
OiOfWPNkQ3eOhEBGr3yhIqts/rayZUh/hmClH293FtNMKfQpPnChXj4LawpIoYRn
fgHQESLnlbnRjpTwxqvwAVtLyqprzBVeEKWOpblw9Nr01jzdtfnQysDGcqB3niIk
syatELXC067E4rh1axfilfd8dHtt5SRE57itZZA5A1gdkYqzhtp09lim6gJIfmmE
EkLXh49J34BJ3OhKhKt+0Xgc6wNuQqB96rvrku4xFkFASr1SdjxyiqGhAfnEvQgj
Z0+mJ1DWH0kAL+npjRTc
=z4pd
-----END PGP SIGNATURE-----