wined3d: GLSL Patch feedback requested
The current cumulative patch is located here: http://cmhousing.net/wine/glsl_cumul.diff (This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet) Before submitting, I plan to fix the following things: - Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?)) This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them: [already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program) Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release() Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up. Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch. Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor) Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0 Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it. I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order.
By the way, here's a comparison screenshot of Civ4 from before my hard drive failed, and this is without having texturing on pixel shaders entirely working. Using GLSL (working completely for vertex shaders 2.0, at least the instructions that Civilization 4 used): http://cmhousing.net/wine/civ4_glsl.png compared to: http://cmhousing.net/wine/civ4_ingame2.png and http://cmhousing.net/wine/civ4_3.png with current git and using regular ARB shaders. So, we at least know that this is going to help. :-) On 6/8/06, Jason Green <jave27(a)gmail.com> wrote:
The current cumulative patch is located here:
http://cmhousing.net/wine/glsl_cumul.diff
(This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet)
Before submitting, I plan to fix the following things:
- Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?))
This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them:
[already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program)
Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release()
Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up.
Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch.
Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor)
Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0
Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0
Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it.
I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order.
That looks very impressive (good progress). I would have to look at the actual GLSL produced by this. The actual translation points look good.
From: "Jason Green" <jave27(a)gmail.com> Date: Thu, 8 Jun 2006 11:54:04 -0400
By the way, here's a comparison screenshot of Civ4 from before my hard drive failed, and this is without having texturing on pixel shaders entirely working. Using GLSL (working completely for vertex shaders 2.0, at least the instructions that Civilization 4 used):
http://cmhousing.net/wine/civ4_glsl.png
compared to:
http://cmhousing.net/wine/civ4_ingame2.png and http://cmhousing.net/wine/civ4_3.png
with current git and using regular ARB shaders.
So, we at least know that this is going to help. :-)
On 6/8/06, Jason Green <jave27(a)gmail.com> wrote:
The current cumulative patch is located here:
http://cmhousing.net/wine/glsl_cumul.diff
(This includes the recently submitted "Split constant loading out of drawPrim()" patch, which hasn't been applied to git yet)
Before submitting, I plan to fix the following things:
- Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) - Switch the shader_reg_maps.constantsF[] to a CHAR array instead of a BOOL array (maybe even a bitmap(?))
This whole patchset should be a no-op for normal users, unless they have the UseGLSL registry key enabled. Here's a list of the changelogs that this cumulative patch entails, somewhat in the order that I'll be sending them:
[already sent] Move constant loading out of DrawPrimDrawStrided() - DrawPrim is just too big of a function. This separates the passing of constants to the shader into new functions. - Fixes an off-by-one error when loading vertex declaration constants (should be <, not <=) - Adds a function for GLSL loading of constants (aka Uniforms) - Adds a GLSL program variable to the stateblock and sets it to 0 (a future patch will actually create this program)
Add GLSL helper functions to device.c - Adds functions to attach & detach shader objects, create and delete programs, and maintain the list of programs. - Adds a list of GLSL shader programs to the device which is initialized on Init3D(), and deleted on Release()
Add the bulk of the GLSL string generation functions - Add a new file glsl_shader.c which contains almost every GLSL specific function we'll need - Move print_glsl_info() into glsl_shader.c - Move the shader_reg_maps struct info into the private header, and make it part of SHADER_OPCODE_ARG. - Create a new shared ps/vs register map for float constants (future patch will make ARB programs use this, too) - This is a big patch, but none of the new functions in glsl_shader.c are being called yet. This just sets them up.
Unified float constant register mapping between ARB pixel and vertex shaders. - Got rid of the separate constant maps. - Side effect of this is that the map is a bit larger for pixel shaders than it needs to be. - Will make this dynamic in a future patch.
Added more declarations to GLSL in generate_glsl_declarations() - Declare more variable names for GLSL programs. - Correct output name for pixel shaders (gl_FragColor instead of glFragColor)
Map pixel shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0
Map vertex shader instructions to GLSL generating functions. - Also, delete the GLSL program when the refcount hits 0
Allow drawPrim to create and use the GLSL program - Now that we can fully create a GLSL program, this patch lets us actually use it.
I would have submitted these all separate for review, but I have some janitorial git cleanup to do first to get all of this in the right order.
- Nick
+ * If a program for the given combination does not exist, create one, and store + * that data in both shader objects so we can delete all of the programs later. + * If it creates a program, it will link the given objects, too. Is this comment still relevant?
- Fix relative addressing (it was working correctly, then I lost some stuff when my hard drive got corrupted, and now it's using R0 instead of A0 as the address register for some reason) For 1.X shaders, the relative address token = 0 (a.k.a R0), and A0.x must be used instead. Another problem may be matrix instructions, which don't seem to set the reladdr token right now.
On Thursday 08 June 2006 17:35, Jason Green wrote:
The current cumulative patch is located here:
I hate you, reviewing all this patch made me crazy. Anyway is an impressive work I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset) And many comments: void inline drawPrimitiveDrawStrided(IWineD3DDevice *iface, BOOL useVertexShaderFunction, BOOL usePixelShaderFunction, int useHW, WineDirect3DVertexStridedData *dataLocations, UINT numberOfvertices, UINT numberOfIndicies, GLenum glPrimType, const void *idxData, short idxSize, int minIndex, long StartIdx) { IWineD3DDeviceImpl *This = (IWineD3DDeviceImpl *)iface; @@ -1791,91 +1920,59 @@ #endif TRACE("Loaded arrays\n"); + /* Bind the correct GLSL shader program based on the currently set vertex & pixel shaders. */ + if (wined3d_settings.shader_mode == SHADER_GLSL) { + GLhandleARB programId; + + set_glsl_shader_program(iface); + programId = This->stateBlock->shaderPrgId; + + if (programId != 0) { + /* Start using this program ID */ + TRACE_(d3d_shader)("Using GLSL program %u\n", programId); + GL_EXTCALL(glUseProgramObjectARB(programId)); + checkGLcall("glUseProgramObjectARB"); + } + } + if (useVertexShaderFunction) { - - It is correct to have GLSL binding code out of "if (useVertexShaderFunction)" block ? - i think moving binding code (GLSL and ARB) on a new base shader method (ex IWineD3DBaseShaderImpl_Bind) will be cleaner (and permit to not have GLS/ARB code in drawprim) - same for shaders constant loading Best Regards, Raphael
On 13/06/06, Raphael <fenix(a)club-internet.fr> wrote:
I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset) Good point :-) I wonder if respecifying the source and recompiling the program is worth it, or if we should just create a new shader object.
- It is correct to have GLSL binding code out of "if (useVertexShaderFunction)" block ? It is, actually. If there's no shader in use, we need to tell GL to use fixed function again. The glUseProgramObjectARB call takes care of that. (programId is zero when no shaders are in use).
On 6/13/06, H. Verbeet <hverbeet(a)gmail.com> wrote:
On 13/06/06, Raphael <fenix(a)club-internet.fr> wrote:
I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset) Good point :-) I wonder if respecifying the source and recompiling the program is worth it, or if we should just create a new shader object.
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/directx9_c/...
From what I can tell (for DX9 at least), the app shouldn't be able to use SetFunction(). I thought it was an internal wined3d function. Unless it's a DX8 call that I'm unaware of. I haven't seen any apps try to do that yet, but maybe I'm just not looking closely enough. :-) The apps tend to CreatePixelShader(), then later [often during the render loop], they SetPixelShader(). I haven't seen any that modify an existing shader yet.
On Tuesday 13 June 2006 18:39, Jason Green wrote:
On 6/13/06, H. Verbeet <hverbeet(a)gmail.com> wrote:
On 13/06/06, Raphael <fenix(a)club-internet.fr> wrote:
I only have one question: i see you have a cache to reuse prgid in set_glsl_shader_program but what happens if user/app change the shaders content (for example using SetFunction) ? your pre-compiled program will be correct ? (because i don't see any list cleanup in shaders function reset)
Good point :-) I wonder if respecifying the source and recompiling the program is worth it, or if we should just create a new shader object.
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/directx9_c /IDirect3DPixelShader9.asp
From what I can tell (for DX9 at least), the app shouldn't be able to
use SetFunction(). I thought it was an internal wined3d function. Unless it's a DX8 call that I'm unaware of. I haven't seen any apps try to do that yet, but maybe I'm just not looking closely enough.
:-) The apps tend to CreatePixelShader(), then later [often during
the render loop], they SetPixelShader(). I haven't seen any that modify an existing shader yet.
No seems correct to me (from DX8-9 APIs) but never say never :) Anyway cleaning glprograms on Delete can be a good idea :) And we must think, for futur, about the ressources manager (was last oliver works). As in some cases the CG will not be able to contains all precompiled shaders couples, and we must swap them on need ;) Best Regards and thx to you two, Raphael
participants (5)
-
H. Verbeet -
Ivan Gyurdiev -
Jason Green -
Nick Burns -
Raphael