On 23 June 2013 21:57, Christian Costa titan.costa@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.
Le 24/06/2013 09:24, Henri Verbeet a écrit :
On 23 June 2013 21:57, Christian Costa titan.costa@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.
2013/6/24 Christian Costa titan.costa@gmail.com:
Le 24/06/2013 09:24, Henri Verbeet a écrit :
On 23 June 2013 21:57, Christian Costa titan.costa@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 unsigned aarg2 : 8; unsigned aarg0 : 8;
+ DWORD constant; + struct color_fixup_desc color_fixup; unsigned tex_type : 3; unsigned dst : 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.
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; elsediff --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 unsigned aarg2 : 8; unsigned aarg0 : 8;
- DWORD constant;
struct color_fixup_desc color_fixup; unsigned tex_type : 3; unsigned dst : 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
On 24 June 2013 16:44, Matteo Bruni matteo.mystral@gmail.com wrote:
@@ -1673,6 +1673,8 @@ struct texture_stage_op unsigned aarg2 : 8; unsigned aarg0 : 8;
- DWORD constant;
- struct color_fixup_desc color_fixup; unsigned tex_type : 3; unsigned dst : 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.
2013/6/25 Henri Verbeet hverbeet@gmail.com
On 24 June 2013 16:44, Matteo Bruni matteo.mystral@gmail.com wrote:
@@ -1673,6 +1673,8 @@ struct texture_stage_op unsigned aarg2 : 8; unsigned aarg0 : 8;
- DWORD constant;
- struct color_fixup_desc color_fixup; unsigned tex_type : 3; unsigned dst : 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.
On 25 June 2013 11:05, Christian Costa titan.costa@gmail.com wrote:
2013/6/25 Henri Verbeet hverbeet@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.
2013/6/25 Henri Verbeet hverbeet@gmail.com
On 25 June 2013 11:05, Christian Costa titan.costa@gmail.com wrote:
2013/6/25 Henri Verbeet hverbeet@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.
-----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.
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 stefandoesinger@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-----
On 25 June 2013 18:26, Stefan Dösinger stefandoesinger@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.
Le 26/06/2013 09:10, Henri Verbeet a écrit :
On 25 June 2013 18:26, Stefan Dösinger stefandoesinger@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.
-
Christian Costa -
Henri Verbeet -
Matteo Bruni -
Stefan Dösinger