3 Jun
2006
3 Jun
'06
6:20 p.m.
Well, I've fixed the speed issues with using GLSL (it's now just as
fast as ARB shaders for the ones that actually work correctly) and
gotten a few other things to work correctly. However, I'm still
having issues with certain pixel/vertex shader combinations. Ohsix
caught on that it may have to do with sampler limits on graphics cards
and how sampling/textures are handled different in GLSL that with ARB.
I'm not sure at the moment and could use some help.
http://cmhousing.net/wine/glsl_hack6.diff
is the diff against the most recent git tree. There are still some
ugly hacks lying around in there. Note that I've separated out most
of the GLSL functions into a new file "glsl_shader.c". Once again, to
use GLSL instead of ARB shaders, you'll have to apply this patch, have
a graphics card that's capable of GLSL, and add the following case
senstive registry key:
HKCU\Software\Wine\Direct3D\UseGLSL = "enabled" (String value)
4 Jun
4 Jun
12:48 a.m.
Hi Jason,
Can you just clarify for my why you are changing the D3DVS_VERSION() and
D3DPS_VERSION() to 3.0 for SHADER_ARB in the caps section of directx.c?
SHADER_ARB refers to ARB_vertex_program and ARB_fragment_program which do
not support features greater than VS 1.1 and PS 1.4 respectively. This code
does not affect SHADER_GLSL at all.
Thanks,
Phil C
Jason Green wrote:
Well, I've fixed the speed issues with using GLSL
(it's now just as
fast as ARB shaders for the ones that actually work correctly) and
gotten a few other things to work correctly. However, I'm still
having issues with certain pixel/vertex shader combinations. Ohsix
caught on that it may have to do with sampler limits on graphics cards
and how sampling/textures are handled different in GLSL that with ARB.
I'm not sure at the moment and could use some help.
http://cmhousing.net/wine/glsl_hack6.diff
is the diff against the most recent git tree. There are still some
ugly hacks lying around in there. Note that I've separated out most
of the GLSL functions into a new file "glsl_shader.c". Once again, to
use GLSL instead of ARB shaders, you'll have to apply this patch, have
a graphics card that's capable of GLSL, and add the following case
senstive registry key:
HKCU\Software\Wine\Direct3D\UseGLSL = "enabled" (String value)
12:57 a.m.
To clarify - It doesn't matter that you're changing the version of the
future software pixel shader support capability but the vertex shader
change is problematic because apps may see the 3.0 VS capability and supply
a 3.0 shader to a 1.4 capable GL implementation.
Phil
Phil Costin wrote:
Hi Jason,
Can you just clarify for my why you are changing the D3DVS_VERSION() and
D3DPS_VERSION() to 3.0 for SHADER_ARB in the caps section of directx.c?
SHADER_ARB refers to ARB_vertex_program and ARB_fragment_program which do
not support features greater than VS 1.1 and PS 1.4 respectively. This
code does not affect SHADER_GLSL at all.
Thanks,
Phil C
3:39 p.m.
On 6/3/06, Phil Costin <philcostin(a)hotmail.com> wrote:
Hi Jason,
Can you just clarify for my why you are changing the D3DVS_VERSION() and
D3DPS_VERSION() to 3.0 for SHADER_ARB in the caps section of directx.c?
Sorry, that snuck in. There are some 2.0 shaders that can be handled
by ARB already, so I was just telling the app that it supported them
as a test. That part shouldn't have been there. :-)
8:20 a.m.
+ /* Declare textures samplers */
+ for(i = 0; i < /*This->baseShader.limits.texture */ 4; i++) {
+// if (reg_maps->texcoord & (1 << i))
+ shader_addline(buffer,"uniform sampler2D mytex%lu;\n", i);
+ }
What's the problem here?
+ if (wined3d_settings.shader_mode == SHADER_GLSL)
+ shader_glsl_add_instruction_modifiers(&hw_arg);
If you choose to pull modifier handling out of the per-opcode function,
this should be done for SHADER_ARB as well, imho.
+ for (i=0; i<max_constants; ++i) {
...
- for (i = 0; i <= WINED3D_VSHADER_MAX_CONSTANTS; ++i) {
Is this fixing off-by-one bug in the old code in the process (or
introducing one)?
+ loadConstants(iface, GL_VERTEX_PROGRAM_ARB, constants,
NULL,
+
vshader->baseShader.limits.constant_float, programId, TRUE);
What's the purpose of the "skip_type_check" parameter - TRUE here.
Doesn't the current code only work for float constants anyway?
+ /* Update the constants */
+ loadConstants(iface, GL_FRAGMENT_PROGRAM_ARB,
This->stateBlock->pixelShaderConstantF,
+ This->stateBlock->pixelShaderConstantT,
pshader->baseShader.limits.constant_float,
+ programId, FALSE);
Those limits aren't checked against GL capabilities yet.
The current 3.0 limit is 224 float constants - does this work properly?
+void shader_glsl_add_instruction_modifiers(SHADER_OPCODE_ARG* arg
Doesn't seem to handle multiple modifiers - I think I changed this
elsewhere recently.
+ FIXME("Unrecognized modifier(0x%#lx)\n", mask >>
D3DSP_DSTMOD_SHIFT);
0x and # are redundant.
+ SHADER_PARAM_FCT_T param_fct;
Please don't add any more fields that do not need to persist into base
shader. Why are two different param functions needed?
+extern void shader_glsl_add_cast(
+ int numSwizzles,
+ char *hwLine);
Where is this defined?
+void pshader_glsl_def(SHADER_OPCODE_ARG* arg) {
...
+ shader->constants[reg & 0xFF] = VS_CONSTANT_CONSTANT;
That's not what the ARBfp code does - pixel shaders are different from
vertex shader (and I don't see why, conceptually this is all the same
thing, only the limit is slightly different).
+ /* Set constants for the temporary argument */
I'm not sure I like how the mnxn functions use a temporary argument
structure and re-invoke the gen. functions. Do they initialize every
field necessary in the arg? I might have added new fields and missed the
initialization part in there - maybe this arg should be 0-ed first with
memcpy.
+ GLhandleARB objList[4]; /* There should never be more than 2
objects attached
+ (one pixel shader and one vertex
shader), but we'll be safe */
This doesn't make sense to me - either you know that there are 2 objects
attached exactly, or you don't. In either case, it's wrong to cover up a
potential bug with a 4-element array.
+ * Note: The downside to this method is that we may end up compiling
and linking
+ * programs that will not be used. Linking is expensive, but it's
typically better to
+ * create more programs than necessary and just bind them when needed
than it is
+ * to create a new program every time you set the shaders
+ */
This is an elaborate scheme with linked lists in each shader - is that
really necessary?
I'm very confused after reading the code (but maybe that's just me :)
Maybe a test case would be useful?
If I call:
X = CreatePixelShader(dev);
Y = CreateVertexShader(dev);
Z = CreateVertexShader(dev);
SetPixelShader(dev, X);
SetVertexShader(dev, Y);
SetVertexShader(dev, Z);
How many programs are being created?
What objects are attached to each?
Which linked lists does each program go into, and
What's the sequence of things being attached/detached, programs
created/destroyed?
4:04 p.m.
On 6/4/06, Ivan Gyurdiev <ivg2(a)cornell.edu> wrote:
+ /* Declare textures samplers */
+ for(i = 0; i < /*This->baseShader.limits.texture */ 4; i++) {
+// if (reg_maps->texcoord & (1 << i))
+ shader_addline(buffer,"uniform sampler2D mytex%lu;\n", i);
+ }
What's the problem here?
A lot of this is just from debugging. The code is still in the proof
of concept phase.
+ if (wined3d_settings.shader_mode == SHADER_GLSL)
+ shader_glsl_add_instruction_modifiers(&hw_arg);
If you choose to pull modifier handling out of the per-opcode function,
this should be done for SHADER_ARB as well, imho.
All of the ARB modifiers are run prior to the command. Whereas, (at
least so far), the GLSL instruction modifiers are necessary to run
after the command.
+ for (i=0; i<max_constants; ++i) {
...
- for (i = 0; i <= WINED3D_VSHADER_MAX_CONSTANTS; ++i) {
Is this fixing off-by-one bug in the old code in the process (or
introducing one)?
Possibly introducing - nice catch.
+ loadConstants(iface,
GL_VERTEX_PROGRAM_ARB, constants,
NULL,
+
vshader->baseShader.limits.constant_float, programId, TRUE);
What's the purpose of the "skip_type_check" parameter - TRUE here.
Doesn't the current code only work for float constants anyway?
It's passing NULL as the type array (because VertexDeclaration
constants are always floats and they don't have one).
+ /* Update the constants */
+ loadConstants(iface, GL_FRAGMENT_PROGRAM_ARB,
This->stateBlock->pixelShaderConstantF,
+ This->stateBlock->pixelShaderConstantT,
pshader->baseShader.limits.constant_float,
+ programId, FALSE);
Those limits aren't checked against GL capabilities yet.
The current 3.0 limit is 224 float constants - does this work properly?
The limits should be checked against the caps in the functions that
set all of the limits based on the shader version. We should probably
do something like set all of the limits as they "should" be, then do a
min() against the GL caps.
+void
shader_glsl_add_instruction_modifiers(SHADER_OPCODE_ARG* arg
Doesn't seem to handle multiple modifiers - I think I changed this
elsewhere recently.
Possibly not. I just haven't hit (or noticed) that test case yet.
+ SHADER_PARAM_FCT_T param_fct;
Please don't add any more fields that do not need to persist into base
shader. Why are two different param functions needed?
This method avoids the base class having to know about its subclasses.
I'm sure there's a better way to do it, still, though.
+ /* Set constants for the temporary argument */
I'm not sure I like how the mnxn functions use a temporary argument
structure and re-invoke the gen. functions. Do they initialize every
field necessary in the arg? I might have added new fields and missed the
initialization part in there - maybe this arg should be 0-ed first with
memcpy.
Agreed.
+ GLhandleARB objList[4]; /* There should
never be more than 2
objects attached
+ (one pixel shader and one vertex
shader), but we'll be safe */
This doesn't make sense to me - either you know that there are 2 objects
attached exactly, or you don't. In either case, it's wrong to cover up a
potential bug with a 4-element array.
Yeah, I must have been tired. :-)
+ * Note: The downside to this method is that we may
end up compiling
and linking
+ * programs that will not be used. Linking is expensive, but it's
typically better to
+ * create more programs than necessary and just bind them when needed
than it is
+ * to create a new program every time you set the shaders
+ */
This is an elaborate scheme with linked lists in each shader - is that
really necessary?
I'm very confused after reading the code (but maybe that's just me :)
If we link the programs together every time the Set__Shader() is
called, it brings performance to a halt (that what my glsl_hack4.diff
did). Everything was roughly 100 times slower that way. So, this
method stores the program for every combination which is used in the
app. That way, when the shaders are set, we already have a linked
program to use. Linking is a very expensive operation. Keeping a
list of them seems like the best method that we could come up with.
If I call:
X = CreatePixelShader(dev);
Y = CreateVertexShader(dev);
Z = CreateVertexShader(dev);
SetPixelShader(dev, X);
SetVertexShader(dev, Y);
SetVertexShader(dev, Z);
How many programs are being created?
3
What objects are attached to each?
Program: Pixel Shader: Vertex Shader:
1 X NULL
2 X Y
3 X Z
Which linked lists does each program go into, and
What's the sequence of things being attached/detached, programs
created/destroyed?
If you run the WINEDEBUG=+d3d_shader trace, you'll get detailed
information about all of the steps involed. Currently, the only time
that the list doesn't get de-allocated is when the app doesn't
properly clean up after itself. So, that will need to be fixed before
this code can be submitted.
On a separate topic, we've learned that the reason that some of these
pixel shaders fail is because I've hard-coded texture2D() and just
assumed that the textures would be 2-dimensional (which is what the
ARB programs do now, too). However, in ARB, only the instructions
that reference the 3D texture are failing. In GLSL, the entire shader
fails if one of the textures is wrong. So, this will have to be
thought out some more, since prior to PS 2.0, the app doesn't have to
declare if it's a 2D or a 3D texture ahead of time.
5:58 p.m.
+ if (wined3d_settings.shader_mode == SHADER_GLSL)
+ shader_glsl_add_instruction_modifiers(&hw_arg);
If you choose to pull modifier handling out of the per-opcode function,
this should be done for SHADER_ARB as well, imho.
All of the ARB modifiers are run prior to the command. Whereas, (at
least so far), the GLSL instruction modifiers are necessary to run
after the command.
Those limits aren't checked against GL capabilities yet.
The current 3.0 limit is 224 float constants - does this work properly?
The limits should be checked against the caps in the functions that
set all of the limits based on the shader version. We should probably
do something like set all of the limits as they "should" be, then do a
min() against the GL caps. +void
shader_glsl_add_instruction_modifiers(SHADER_OPCODE_ARG* arg
Doesn't seem to handle multiple modifiers - I think I changed this
elsewhere recently.
Possibly not. I just haven't hit (or noticed) that test case yet. + SHADER_PARAM_FCT_T param_fct;
Please don't add any more fields that do not need to persist into base
shader. Why are two different param functions needed?
This method avoids the base class having to know about its subclasses. This is an
elaborate scheme with linked lists in each shader - is that
really necessary?
I'm very confused after reading the code (but maybe that's just me :)
If we link the programs together every time the Set__Shader() is
called, it brings performance to a halt (that what my glsl_hack4.diff
did). Everything was roughly 100 times slower that way. So, this
method stores the program for every combination which is used in the
app. That way, when the shaders are set, we already have a
linked
program to use. Linking is a very expensive operation. Keeping a
list of them seems like the best method that we could come up with.
Ok... I guess
maybe it's necessary.
On a separate topic, we've learned that the reason
that some of these
pixel shaders fail is because I've hard-coded texture2D() and just
assumed that the textures would be 2-dimensional (which is what the
ARB programs do now, too).
However, in ARB, only the instructions
that reference the 3D texture are failing. In GLSL, the entire shader
fails if one of the textures is wrong. So, this will have to be
thought out some more, since prior to PS 2.0, the app doesn't have to
declare if it's a 2D or a 3D texture ahead of time.
Yes, I'm not really
sure how this is handled prior to ps2, but surely
there's tutorials online that explain this. Maybe the shader does its
own per-type processing using those texreg2ar, and other instructions
(tex matrix instructions?). For 2.0+ I can send you a patch, which
probably works, but needs more testing. It stores the sampler types in
the SHADER_OPCODE_ARG - I just changed the hw_tex function a little bit.
7:34 p.m.
On 04/06/06, Ivan Gyurdiev <ivg2(a)cornell.edu> wrote:
Well, it seems to store a bit more than that. If I
create pshaders A,B,
and vshaders C,D, and do:
set_pixel(A); set_vertex(C);
draw_stuff();
set_pixel(B); set_vertex(D);
draw_stuff();
set_pixel(NULL), set_vertex(NULL)
Won't this create...5 different programs, all kept in memory for the
entire lifetime of the program, while we only need 2 [1 active]?
(A, NULL), (A,C), (B,C), (B,D), (NULL, D)
I think it should be possible to change
the code in such a way that
the programs are only created once they're actually used.
Also, is it necessary to store link pair info in both
shaders - isn't
this redundant somehow?
Well, yes. When a shader is released, the programs it's
linked to
should be destroyed.
However, it's probably not neccesary to point to the other shader. We
could probably just link to a program struct that contains the program
id and a list of linked shaders. It doubt it will make much of a
difference wrt amount of memory used though, at least not as long as
there are only two shaders involved. On the other hand, for d3d10
we'll also need geometry / primitive shaders.
6 Jun
6 Jun
11:31 a.m.
On Sun, 04 Jun 2006 12:04:21 -0400, Jason Green wrote:
Re: the recent discussion on code commenting and documentation, it'd be
nice if the patch explained this somewhere. It'd be even nicer still if at
some point one of you guys could take a break from the codeface and write
a chapter for the Wine Developer Guide on shaders and how all this works
... it seems incredibly opaque and the last thing we want is a re-run of
the DCOM situation where the only people who understood it left or hacked
on other things for a while and we lost the knowledge of how it worked.
thanks -mike
This is an
elaborate scheme with linked lists in each shader - is that
really necessary?
I'm very confused after reading the code (but maybe that's just me :)
If we link the programs together every time the Set__Shader() is called,
it brings performance to a halt (that what my glsl_hack4.diff did).
5:06 p.m.
On 6/6/06, Mike Hearn <mike(a)plan99.net> wrote:
On Sun, 04 Jun 2006 12:04:21 -0400, Jason Green
wrote:
Re: the recent discussion on code commenting and documentation, it'd be
nice if the patch explained this somewhere. It'd be even nicer still if at
some point one of you guys could take a break from the codeface and write
a chapter for the Wine Developer Guide on shaders and how all this works
Yep, that is all planned before I submit any of the new real work to
wine-patches. I'm still trying to figure out exactly where some of
this stuff needs to go and what pitfalls we'll be up against. Should
make it easier for Alexandre so he doesn't have to apply 800 patches /
month of us moving stuff around after the fact. :-)
I'll add a section to the DirectX-Shaders wiki page with an
explanation soon, as well as make the comments in the source code more
verbose (and/or link to the wiki where applicable).
5:55 p.m.
On 6/6/06, Jason Green <jave27(a)gmail.com> wrote:
I'll add a section to the DirectX-Shaders wiki
page with an
explanation soon, as well as make the comments in the source code more
verbose (and/or link to the wiki where applicable).
FYI - I just added a bunch to this page to get us started:
http://wiki.winehq.org/DirectX-Shaders
11 Jun
11 Jun
8:24 p.m.
On Tue, 06 Jun 2006 13:55:15 -0400, Jason Green wrote:
FYI - I just added a bunch to this page to get us
started:
http://wiki.winehq.org/DirectX-Shaders
That's awesome, exactly the sort of thing I was looking for. This sort of
high level overview is very useful for new hackers! :)
thanks -mike
12 Jun
12 Jun
11:57 p.m.
On Sunday 11 June 2006 22:24, Mike Hearn wrote:
On Tue, 06 Jun 2006 13:55:15 -0400, Jason Green
wrote:
:)
Well it will not be suficient to begin hacking on wined3d
(as only wined3d shaders architecture is explained)
Anyway Most of code is splitted in "logical" sources ... and documented when
needed:
for shaders/declaration:
baseshader.c, glsl_shader.c, pixelshader.c, vertexshader.c,
vertexdeclaration.c
for textures:
basetexture.c, cubetexture.c, texture.c, volumetexture.c
for surfaces:
surface.c, surface_gdi.c,
others who have d3d compliant names: :)
device.c, directx.c, drawprim.c, indexbuffer.c, palette.c, query.c,
resource.c, stateblock.c, swapchain.c, utils.c, vertexbuffer.c, volume.c,
wined3d_main.c
Only one thing to new hackers: never begin with device.c :)
Regards,
Raphael
FYI - I just added a bunch to this page to get us
started:
http://wiki.winehq.org/DirectX-Shaders
That's awesome, exactly the sort of thing I was looking for. This sort of
high level overview is very useful for new hackers! :)
6549
days inactive
6559
days old
13 comments
6 participants
participants (6)
-
H. Verbeet -
Ivan Gyurdiev -
Jason Green -
Mike Hearn -
Phil Costin -
Raphael