Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
On 25 June 2013 18:26, Stefan Dösinger stefandoesin...@gmail.com wrote: While I agree with Henri's statement to some extend - patch reviewing is a terribly exhausting task - I think the statement was a bit harsh to say to someone who donates his personal time to the project. Well, I could try to sugarcoat it a bit, and add in all the little nuances, about how e.g. a patch that's otherwise great isn't going to be rejected because of some minor formatting issue, or how someone that's been with the project for a while would be expected to already know these things, while a new contributor would perhaps be given some more patience. But that's really the basic concept. And it's not a particularly new concept either, or all that specific to Wine.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
Le 26/06/2013 09:10, Henri Verbeet a écrit : On 25 June 2013 18:26, Stefan Dösinger stefandoesin...@gmail.com wrote: While I agree with Henri's statement to some extend - patch reviewing is a terribly exhausting task - I think the statement was a bit harsh to say to someone who donates his personal time to the project. Well, I could try to sugarcoat it a bit, and add in all the little nuances, about how e.g. a patch that's otherwise great isn't going to be rejected because of some minor formatting issue, or how someone that's been with the project for a while would be expected to already know these things, while a new contributor would perhaps be given some more patience. But that's really the basic concept. And it's not a particularly new concept either, or all that specific to Wine. There are good reasons to have someone else do the review or developped tests. Anyway choking on a typo is a bit exaggerated. The main issue would have been enough. Someone else did it so that's fine.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
On 24 June 2013 16:44, Matteo Bruni matteo.myst...@gmail.com wrote: @@ -1673,6 +1673,8 @@ struct texture_stage_op unsignedaarg2 : 8; unsignedaarg0 : 8; +DWORD constant; + struct color_fixup_desc color_fixup; unsignedtex_type : 3; unsigneddst : 1; You don't need this if you use uniforms. Also adding a test would be nice probably. Yeah, that's the main issue. We don't want a shader for every possible set of per-stage constant values. The one you missed is that if we implement this, we should also set the appropriate caps bit. The most obvious issue is of course the subject line. Sure, typos happen, but it's just plain sloppy. At least try to pretend you put some effort into making sure the patch is as good as it can be before submitting it.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
2013/6/25 Henri Verbeet hverb...@gmail.com On 24 June 2013 16:44, Matteo Bruni matteo.myst...@gmail.com wrote: @@ -1673,6 +1673,8 @@ struct texture_stage_op unsignedaarg2 : 8; unsignedaarg0 : 8; +DWORD constant; + struct color_fixup_desc color_fixup; unsignedtex_type : 3; unsigneddst : 1; You don't need this if you use uniforms. Also adding a test would be nice probably. Yeah, that's the main issue. We don't want a shader for every possible set of per-stage constant values. The one you missed is that if we implement this, we should also set the appropriate caps bit. Spin Tires does not seem to check this caps. That's why I missed it first. The most obvious issue is of course the subject line. Sure, typos happen, but it's just plain sloppy. It's a typo yes. You can easy deduced this with the patch description. At least try to pretend you put some effort into making sure the patch is as good as it can be before submitting it. I could have send the patch to wine-devel first but would I get something longer that several issues as feedback? I can certainly improve my patches and you can certainly improve your way of giving feedback but that would be great if you could avoid this kind of commentary. This is not usefull. At least for me. For technical stuff you can be as verbose as you want. I'm full open.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
On 25 June 2013 11:05, Christian Costa titan.co...@gmail.com wrote: 2013/6/25 Henri Verbeet hverb...@gmail.com At least try to pretend you put some effort into making sure the patch is as good as it can be before submitting it. I could have send the patch to wine-devel first but would I get something longer that several issues as feedback? No, you're missing the point. Why do you expect me to even look at a patch if you clearly didn't even check for obvious typos yourself? Bottom line, if you want me to spend time reviewing a patch and giving feedback, make sure it's worth that time.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
2013/6/25 Henri Verbeet hverb...@gmail.com On 25 June 2013 11:05, Christian Costa titan.co...@gmail.com wrote: 2013/6/25 Henri Verbeet hverb...@gmail.com At least try to pretend you put some effort into making sure the patch is as good as it can be before submitting it. I could have send the patch to wine-devel first but would I get something longer that several issues as feedback? No, you're missing the point. Why do you expect me to even look at a patch if you clearly didn't even check for obvious typos yourself? Bottom line, if you want me to spend time reviewing a patch and giving feedback, make sure it's worth that time. Are we really discussing about a typo in the subject line? Implement just comes naturally from the fixme when I create an empty patch. It turned out after coding that Fix is more appropriate. I forgot to updated it. This will be fix. According to the feedback I got, altough the patch is not correct, it doesn't seem it does not worth looking at. And now there is materials to make it better.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 2013-06-25 18:08, schrieb Christian Costa: Are we really discussing about a typo in the subject line? Implement just comes naturally from the fixme when I create an empty patch. It turned out after coding that Fix is more appropriate. I forgot to updated it. This will be fix. I think the typo Henri refers to is fonction, which should be function. While I agree with Henri's statement to some extend - patch reviewing is a terribly exhausting task - I think the statement was a bit harsh to say to someone who donates his personal time to the project. I'll try to do a better job looking at patches myself to help weed out the most obvious mistakes - right now he's doing all the d3d reviewing work. -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRycSuAAoJEN0/YqbEcdMwOHwP/R98Fs+ZunKuRIPnenvXFEat hP3C1JCrGhxANiYIggkdcvfjPxCSbgz+LM4nfZC4qARSHnYzvxhcQxALN2je/lwb 1iWx/xZ0Czy2yR3rHqJCEW0jXkV53O/Lkqbs/Nf2ue0I7vJWuhaA3Tl+P1esE3m8 rdeRQY89SyXuvfZGtO1tPnGwsYMU/NuSoIffKkLjjWpBhScvJmcSUYOL9RuFk6Tj +r7zGp9niSfVO9N8JY7BTAlXTcAK9p0gKaWzAuCXYFr8gcbOaGB3kDSsbIhtHe57 8oWEDA9pn3tO7l1fXGZdbyfamIPyjg5boh1yHireDze9eefGHrd/WQqEu1xqVd95 BVlFUt2nMPZoR9A0MUT3/0kbq/sfJ05ytCTZ5AlbNAO8msA0fTX58gw6UA9QXrET 1dpE+tjGHzN49qjcBtUmFUII6zc+m8B91Xlhplupda2v/TZv+tzQvrsMIuwRvLvP hTzxaZo/kFr2Pnlx8Sgk0ImNUfi9AIXpLo/h7geLgMi0kMTzkHY4qW2LGf74GNhU NtBgN+4Y1iFLeK6dbKIenD75MN/JGOp6eOHK53E2++RykLxJLyqCTPqplShE/Zz7 hnt80SQTR0jYttA9KsHGE3GnqKPc8RndMnjGnMCyNhLj9Jm27te2VMGLbw4mK0yD C+nOcltXE3Hu3AYpzEXd =C1yJ -END PGP SIGNATURE-
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
Thanks Stefan! No problem with me. I don't take it personnally. :) I'm not statisfied with the submission/review with some criterias which are sometimes rules, sometimes tastes, with patches (not necessarily mine) that remain in the new state without any comments while another with a missing dot would have more attention. I live with that, I'm used to enough, but I'm wondering how newcomers that are not paid could contribute. Fortunately there are cool guys. Not necessary overskilled but who try to do their humble best. This is what gives some fun for me. :) BTW, fonction is the french word for function. Probably a wrong locale. ;) 2013/6/25 Stefan Dösinger stefandoesin...@gmail.com -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 2013-06-25 18:08, schrieb Christian Costa: Are we really discussing about a typo in the subject line? Implement just comes naturally from the fixme when I create an empty patch. It turned out after coding that Fix is more appropriate. I forgot to updated it. This will be fix. I think the typo Henri refers to is fonction, which should be function. While I agree with Henri's statement to some extend - patch reviewing is a terribly exhausting task - I think the statement was a bit harsh to say to someone who donates his personal time to the project. I'll try to do a better job looking at patches myself to help weed out the most obvious mistakes - right now he's doing all the d3d reviewing work. -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.20 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRycSuAAoJEN0/YqbEcdMwOHwP/R98Fs+ZunKuRIPnenvXFEat hP3C1JCrGhxANiYIggkdcvfjPxCSbgz+LM4nfZC4qARSHnYzvxhcQxALN2je/lwb 1iWx/xZ0Czy2yR3rHqJCEW0jXkV53O/Lkqbs/Nf2ue0I7vJWuhaA3Tl+P1esE3m8 rdeRQY89SyXuvfZGtO1tPnGwsYMU/NuSoIffKkLjjWpBhScvJmcSUYOL9RuFk6Tj +r7zGp9niSfVO9N8JY7BTAlXTcAK9p0gKaWzAuCXYFr8gcbOaGB3kDSsbIhtHe57 8oWEDA9pn3tO7l1fXGZdbyfamIPyjg5boh1yHireDze9eefGHrd/WQqEu1xqVd95 BVlFUt2nMPZoR9A0MUT3/0kbq/sfJ05ytCTZ5AlbNAO8msA0fTX58gw6UA9QXrET 1dpE+tjGHzN49qjcBtUmFUII6zc+m8B91Xlhplupda2v/TZv+tzQvrsMIuwRvLvP hTzxaZo/kFr2Pnlx8Sgk0ImNUfi9AIXpLo/h7geLgMi0kMTzkHY4qW2LGf74GNhU NtBgN+4Y1iFLeK6dbKIenD75MN/JGOp6eOHK53E2++RykLxJLyqCTPqplShE/Zz7 hnt80SQTR0jYttA9KsHGE3GnqKPc8RndMnjGnMCyNhLj9Jm27te2VMGLbw4mK0yD C+nOcltXE3Hu3AYpzEXd =C1yJ -END PGP SIGNATURE-
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
On 23 June 2013 21:57, Christian Costa titan.co...@gmail.com wrote: When D3DTA_CONSTANT is use in a texture stage, the generated shader uses variables that are not defined making thus the compilation to fail. This patch declare these variables with the value from the related texture stage state D3D_TSS_CONSTANT. This fixes the text display in Spin Tires demo. This patch has several issues. Even without those, it should probably be deferred anyway.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
Le 24/06/2013 09:24, Henri Verbeet a écrit : On 23 June 2013 21:57, Christian Costa titan.co...@gmail.com wrote: When D3DTA_CONSTANT is use in a texture stage, the generated shader uses variables that are not defined making thus the compilation to fail. This patch declare these variables with the value from the related texture stage state D3D_TSS_CONSTANT. This fixes the text display in Spin Tires demo. This patch has several issues. Even without those, it should probably be deferred anyway. Even deferred, can you elaborate a bit so I can fix them. Unless you have already something.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
2013/6/24 Christian Costa titan.co...@gmail.com:
Le 24/06/2013 09:24, Henri Verbeet a écrit :
On 23 June 2013 21:57, Christian Costa titan.co...@gmail.com wrote:
When D3DTA_CONSTANT is use in a texture stage, the generated shader uses
variables that are not defined making thus the compilation to fail.
This patch declare these variables with the value from the related
texture stage state D3D_TSS_CONSTANT.
This fixes the text display in Spin Tires demo.
This patch has several issues. Even without those, it should probably
be deferred anyway.
Even deferred, can you elaborate a bit so I can fix them. Unless you have
already something.
I'm not Henri but I can mention a number of issues (which might or
might not match with Henri's).
+for (stage = 0; stage MAX_TEXTURES settings-op[stage].cop
!= WINED3D_TOP_DISABLE; ++stage)
You probably want to generate this code only for texture stages
actually using constants.
+{
+float constant[4];
+
+constant[0] = ((settings-op[stage].constant 16) 0xff) / 255.0f;
+constant[1] = ((settings-op[stage].constant 8) 0xff) / 255.0f;
+constant[2] = ( settings-op[stage].constant 0xff) / 255.0f;
+constant[3] = ((settings-op[stage].constant 24) 0xff) / 255.0f;
No need to open code it, the macro D3DCOLORTOGLFLOAT4 does just that.
+
+shader_addline(buffer, const vec4 const%d = , stage);
+shader_glsl_append_imm_vec4(
buffer, constant);
+shader_addline(buffer, ;\n);
I assume it would be better to make these proper uniforms and update
their value in shader_glsl_load_constants() instead.
diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c
index 5b7fb3c..083a0d7 100644
--- a/dlls/wined3d/utils.c
+++ b/dlls/wined3d/utils.c
@@ -3312,6 +3312,8 @@ void gen_ffp_frag_op(const struct
wined3d_context *context, const struct wined3d
settings-op[i].aarg1 = aarg1;
settings-op[i].aarg2 = aarg2;
+settings-op[i].constant = state-texture_states[i][
WINED3D_TSS_CONSTANT];
+
if (state-texture_states[i][WINED3D_TSS_RESULT_ARG] == WINED3DTA_TEMP)
settings-op[i].dst = tempreg;
else
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h
index dca0d61..c1d6c91 100644
--- a/dlls/wined3d/wined3d_private.h
+++ b/dlls/wined3d/wined3d_private.h
@@ -1673,6 +1673,8 @@ struct texture_stage_op
unsignedaarg2 : 8;
unsignedaarg0 : 8;
+DWORD constant;
+
struct color_fixup_desc color_fixup;
unsignedtex_type : 3;
unsigneddst : 1;
You don't need this if you use uniforms.
Also adding a test would be nice probably.
That said, maybe Henri already has a patch for it.
Re: [PATCH] wined3d: Implement per-stage constant in glsl fixed fonction pipeline.
I'm not Henri but I can mention a number of issues (which might or
might not match with Henri's).
+for (stage = 0; stage MAX_TEXTURES settings-op[stage].cop
!= WINED3D_TOP_DISABLE; ++stage)
You probably want to generate this code only for texture stages
actually using constants.
Yes. I was thinking about that first but chose the easiest way. Will fix.
+{
+float constant[4];
+
+constant[0] = ((settings-op[stage].constant 16) 0xff) / 255.0f;
+constant[1] = ((settings-op[stage].constant 8) 0xff) / 255.0f;
+constant[2] = ( settings-op[stage].constant 0xff) / 255.0f;
+constant[3] = ((settings-op[stage].constant 24) 0xff) / 255.0f;
No need to open code it, the macro D3DCOLORTOGLFLOAT4 does just that.
Ok. I will sue it.
+
+shader_addline(buffer, const vec4 const%d = , stage);
+shader_glsl_append_imm_vec4(
buffer, constant);
+shader_addline(buffer, ;\n);
I assume it would be better to make these proper uniforms and update
their value in shader_glsl_load_constants() instead.
I was wondering when the shader was regenerated and how changing
constant would affect it.
Indeed this seems more correct.
diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c
index 5b7fb3c..083a0d7 100644
--- a/dlls/wined3d/utils.c
+++ b/dlls/wined3d/utils.c
@@ -3312,6 +3312,8 @@ void gen_ffp_frag_op(const struct
wined3d_context *context, const struct wined3d
settings-op[i].aarg1 = aarg1;
settings-op[i].aarg2 = aarg2;
+settings-op[i].constant = state-texture_states[i][
WINED3D_TSS_CONSTANT];
+
if (state-texture_states[i][WINED3D_TSS_RESULT_ARG] ==
WINED3DTA_TEMP)
settings-op[i].dst = tempreg;
else
diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h
index dca0d61..c1d6c91 100644
--- a/dlls/wined3d/wined3d_private.h
+++ b/dlls/wined3d/wined3d_private.h
@@ -1673,6 +1673,8 @@ struct texture_stage_op
unsignedaarg2 : 8;
unsignedaarg0 : 8;
+DWORD constant;
+
struct color_fixup_desc color_fixup;
unsignedtex_type : 3;
unsigneddst : 1;
You don't need this if you use uniforms.
Yes indeed.
Also adding a test would be nice probably.
I will try to do so if it's not too hard altough I guess there're
probably some template I can reuse in the existing tests.
That said, maybe Henri already has a patch for it.
If Henri or Stefan have already a patch for it or plan to fix it once
Wine 1.6 is released I can drop my patch otherwise I don't mind fix it
to do things the right way.
Thanks for you feedback
Christian
