Re: [10/10] WineD3D: Sort out some limit confusion
2008/8/22 Stefan Dösinger <stefan(a)codeweavers.com>:
+ glGetIntegerv(GL_MAX_TEXTURE_COORDS, &gl_max); + gl_info->max_texture_coords = min(MAX_TEXTURES, gl_max); + TRACE_(d3d_caps)("Max texture coords: %d\n", gl_info->max_texture_coords); I don't know if this is intentional or not, but did you mean to use gl_info->max_textures here?
@@ -3172,7 +3172,7 @@ static void loadTexCoords(IWineD3DStateBlockImpl *stateblock, WineDirect3DVertex return; }
- for (textureNo = 0; textureNo < GL_LIMITS(texture_stages); ++textureNo) { + for (textureNo = 0; textureNo < GL_LIMITS(texture_coords); ++textureNo) { int coordIdx = stateblock->textureState[textureNo][WINED3DTSS_TEXCOORDINDEX];
mapped_stage = stateblock->wineD3DDevice->texUnitMap[textureNo]; Shouldn't that be testing mapped_stage against GL_LIMITS(texture_coords) instead?
- if (GL_SUPPORT(NV_REGISTER_COMBINERS)) { - /* The number of the mapped stages increases monotonically, so it's fine to use the last used one */ - for (textureNo = mapped_stage + 1; textureNo < GL_LIMITS(textures); ++textureNo) { - GL_EXTCALL(glMultiTexCoord4fARB(GL_TEXTURE0_ARB + textureNo, 0, 0, 0, 1)); - } - } I'm probably missing something here, but why is it safe to remove that?
@@ -420,7 +420,7 @@ static void drawStridedSlow(IWineD3DDevice *iface, WineDirect3DVertexStridedData }
/* Texture coords --------------------------- */ - for (textureNo = 0; textureNo < GL_LIMITS(texture_stages); ++textureNo) { + for (textureNo = 0; textureNo < GL_LIMITS(texture_coords); ++textureNo) {
if (!GL_SUPPORT(ARB_MULTITEXTURE) && textureNo > 0) { FIXME("Program using multiple concurrent textures which this opengl implementation doesn't support\n"); Similar to above, shouldn't this test texture_idx against GL_LIMITS(texture_coords) instead of textureNo?
In the bigger picture, these various limits are pretty tricky to get right, I wonder if it might make more sense to handle most of this stuff in the tex unit map instead. That way we could be sure that if a stage is mapped to a sampler that sampler is safe to use.
2008/8/23 Henri Verbeet <hverbeet(a)gmail.com>:
2008/8/22 Stefan Dösinger <stefan(a)codeweavers.com>:
+ glGetIntegerv(GL_MAX_TEXTURE_COORDS, &gl_max); + gl_info->max_texture_coords = min(MAX_TEXTURES, gl_max); + TRACE_(d3d_caps)("Max texture coords: %d\n", gl_info->max_texture_coords); I don't know if this is intentional or not, but did you mean to use gl_info->max_textures here? Just to clarify, "instead of MAX_TEXTURES".
2008/8/22 Stefan Dösinger <stefan(a)codeweavers.com>:
+ glGetIntegerv(GL_MAX_TEXTURE_COORDS, &gl_max); + gl_info->max_texture_coords = min(MAX_TEXTURES, gl_max); + TRACE_(d3d_caps)("Max texture coords: %d\n", gl_info- max_texture_coords); I don't know if this is intentional or not, but did you mean to use gl_info->max_textures here? No, I meant to clamp it by the constant MAX_TEXTURES(=8). gl_info->max_textures is wrong here because gl_info->max_textures could be 4, GL_MAX_TEXTURE_COORDS could be 8. If the application uses fixed function vertex processing and pixel shaders it can make use of all 8 texture coords in the vertex pipeline even though the fixed function fragment pipeline supports only 4 textures. Or we could use the arbfp code which isn't limited by gl_info->max_textures.
@@ -3172,7 +3172,7 @@ static void loadTexCoords(IWineD3DStateBlockImpl *stateblock, WineDirect3DVertex return; }
- for (textureNo = 0; textureNo < GL_LIMITS(texture_stages); ++textureNo) { + for (textureNo = 0; textureNo < GL_LIMITS(texture_coords); ++textureNo) { int coordIdx = stateblock- textureState[textureNo][WINED3DTSS_TEXCOORDINDEX];
mapped_stage = stateblock->wineD3DDevice- texUnitMap[textureNo]; Shouldn't that be testing mapped_stage against GL_LIMITS(texture_coords) instead? You mean instead of textureNo? I don't think so. A stage is always mapped to a lower one, so I think we cannot trigger an error here. Dealing with all supported texture units here spares the loop below.
- if (GL_SUPPORT(NV_REGISTER_COMBINERS)) { - /* The number of the mapped stages increases monotonically, so it's fine to use the last used one */ - for (textureNo = mapped_stage + 1; textureNo < GL_LIMITS(textures); ++textureNo) { - GL_EXTCALL(glMultiTexCoord4fARB(GL_TEXTURE0_ARB + textureNo, 0, 0, 0, 1)); - } - } I'm probably missing something here, but why is it safe to remove that? Because we have handled all supported texture units in the loop above. and supplied it with default coordinates if needed.
In the bigger picture, these various limits are pretty tricky to get right, I wonder if it might make more sense to handle most of this stuff in the tex unit map instead. That way we could be sure that if a stage is mapped to a sampler that sampler is safe to use. While this sounds nice I am afraid it won't work too well. Beyond specifying the new settings we have to do other things like unsetting unneeded states, etc, so I don't think we can keep the state mappings all in one place. My other concern is that different pipeline implementations need different limits.
2008/8/24 Stefan Dösinger <stefan(a)codeweavers.com>:
No, I meant to clamp it by the constant MAX_TEXTURES(=8). gl_info->max_textures is wrong here because gl_info->max_textures could be 4, GL_MAX_TEXTURE_COORDS could be 8. If the application uses fixed function vertex processing and pixel shaders it can make use of all 8 texture coords in the vertex pipeline even though the fixed function fragment pipeline supports only 4 textures. Or we could use the arbfp code which isn't limited by gl_info->max_textures.
Ok, I wasn't sure. Wrt arbfp though, it should report 8 for max_textures in that case, because that's how much simultaneous textures d3d can address. max_textures, max_texture_stages, etc. are the limits for what our d3d implementation can do, not GL.
You mean instead of textureNo? I don't think so. A stage is always mapped to a lower one, so I think we cannot trigger an error here. Dealing with all supported texture units here spares the loop below.
You probably can't trigger an error, but you might miss coordinates for a high texture stage that's mapped to a lower texture unit. I also don't think we should rely on details of how stages are mapped to units if we can avoid it.
- if (GL_SUPPORT(NV_REGISTER_COMBINERS)) { - /* The number of the mapped stages increases monotonically, so it's fine to use the last used one */ - for (textureNo = mapped_stage + 1; textureNo < GL_LIMITS(textures); ++textureNo) { - GL_EXTCALL(glMultiTexCoord4fARB(GL_TEXTURE0_ARB + textureNo, 0, 0, 0, 1)); - } - } I'm probably missing something here, but why is it safe to remove that? Because we have handled all supported texture units in the loop above. and supplied it with default coordinates if needed.
Except that we haven't. We've looped through the first max_texture_coords texture stages, but there's not guarantee that those stages also touch the first max_texture_coords units. In fact, unless you're using a 1:1 mapping, it's guaranteed they don't.
In the bigger picture, these various limits are pretty tricky to get right, I wonder if it might make more sense to handle most of this stuff in the tex unit map instead. That way we could be sure that if a stage is mapped to a sampler that sampler is safe to use. While this sounds nice I am afraid it won't work too well. Beyond specifying the new settings we have to do other things like unsetting unneeded states, etc, so I don't think we can keep the state mappings all in one place. My other concern is that different pipeline implementations need different limits.
If you can guarantee that a texture unit isn't mapped when it isn't used, you can just walk through the rev_tex_unit_map and find the corresponding texture stage. That makes much more sense from GL's point of view.
Ok, I wasn't sure. Wrt arbfp though, it should report 8 for max_textures in that case, because that's how much simultaneous textures d3d can address. max_textures, max_texture_stages, etc. are the limits for what our d3d implementation can do, not GL. No, I don't think so. Even if we're using ARBfp, calling a glEnable(GL_TEXTURE_2D) in texture unit 5 is not valid on e.g. nvidia cards. So we can't put the arbfp limit into the glinfo->max_textures member.
In some places, e.g. the texture unit mapping code, we want to use the pipeline implementation limit instead of the opengl fixed function limit. I have a different patch doing that. I'll look at your other points later today, I don't have a quick answer for them right now.
2008/8/24 Stefan Dösinger <stefan(a)codeweavers.com>:
Ok, I wasn't sure. Wrt arbfp though, it should report 8 for max_textures in that case, because that's how much simultaneous textures d3d can address. max_textures, max_texture_stages, etc. are the limits for what our d3d implementation can do, not GL. No, I don't think so. Even if we're using ARBfp, calling a glEnable(GL_TEXTURE_2D) in texture unit 5 is not valid on e.g. nvidia cards. Sure, but there should be no reason to make that call either. In pretty much every place GL_LIMITS(textures) is used, it would either need to be replaced with the fragment pipe implementation based limit or not be executed at all (eg. code that calls glTexEnvf or creates dummy textures really doesn't need to be executed when using arbfp). The only possible exception is SetupForBlit(), but I'm tempted to say we shouldn't be using that either when shaders are available. It's a pretty minor point though, and I'm not necessarily opposed to a separate fragment pipe based limit.
Sure, but there should be no reason to make that call either. In pretty much every place GL_LIMITS(textures) is used, it would either need to be replaced with the fragment pipe implementation based limit or not be executed at all (eg. code that calls glTexEnvf or creates dummy textures really doesn't need to be executed when using arbfp). The only possible exception is SetupForBlit(), but I'm tempted to say we shouldn't be using that either when shaders are available. It's a pretty minor point though, and I'm not necessarily opposed to a separate fragment pipe based limit. Yes, we can eliminate those calls, and currently SetupForBlit and the dummy textures are the only remaining cases. However, I prefer to keep all the limits in the structures, since replacing them based on the pipeline implementation will be messy.
In the end I think "Why is this code reading the wrong limit" is easier to debug than "Why does this limit contain the wrong value"
Hmm. Does the application use a fixed function texture coordinate specified with D3DECLUSAGE_FLOAT4?
It should be simple to support this. Essentially the "int coordsToUse = sd->u.s.texCoords[coordIdx].dwType + 1" is wrong, this needs a better mapping:
decltype: coordsToUse: D3DDECLTYPE_FLOAT1 1 D3DDECLTYPE_FLOAT2 2 D3DDECLTYPE_FLOAT3 3 D3DDECLTYPE_FLOAT4 4 D3DDECLTYPE_D3DCOLOR 4 D3DDECLTYPE_UBYTE4 4 D3DDECLTYPE_SHORT2 2 D3DDECLTYPE_SHORT4 4 D3DDECLTYPE_UBYTE4N 4 D3DDECLTYPE_SHORT2N 2 D3DDECLTYPE_SHORT4N 4 D3DDECLTYPE_USHORT2N 2 D3DDECLTYPE_USHORT4N 4 D3DDECLTYPE_UDEC3 ??? D3DDECLTYPE_DEC3N ??? D3DDECLTYPE_FLOAT16_2 2 D3DDECLTYPE_FLOAT16_4 4
Also the function calls below shouldn't be selected based on a switch-case statement, but rather on the declaration itself. q, r, t and s should be read in a different way too. The system used for providing other parameters like diffuse_funcs sounds sane here, although some extra care is needed to keep working without GL_ARB_multitexture support.
Also I strongly recommend writing a test here. Generally, fixed function attributes with non-standard data types are spooky on Windows, so the real problem may be somewhere else.
I could not answer that it uses a drawprim float 4 before it calls drawprim with the 7 parm. I started to work on a better fix for this rather than the one where I just added the extra values in. But the extra values seemed to solve it. While I am not sure its behavior on windows calling the same call (whatever that call is). Correct in some cases the vertex is a short or a byte that must be normalized. However I am assuming or assumed that that is done by the application calling the API and not the actual API itself. This assumption seems to work but not 100% sure as I don't know how it works on windows. The DEC 3 and DEC3N specify 3 variables q,r,t with the 4th being I think the link said a 1. The N specifies if the values are normalized. Chris
participants (3)
-
Chris Ahrendt -
Henri Verbeet -
Stefan Dösinger