I just have a couple wined3d code questions/comments that I'd like to clear up and possible submit patches if my understanding is correct. If I am misunderstanding the code please correct me.
1) basetexture.c:BindTexture() Just _before_ we call glBindTexture we call glTexParameteri and either set GL_TEXTURE_MAX_LEVEL or GL_TEXTURE_MIN_FILTER. The comment in the code is:
"Always need to reset the number of mipmap levels when rebinding as it is a property of the active texture unit, and another texture may have set it to a different value"
However, this doesn't make any sense to me and I believe it to be incorrect. glTexParameter affects the currently bound texture object. If the current and next texture to be bound have different mipmaps levels or parameters it should not be necessary to change these settings as all the state is encapsulated correctly in the OpenGL texture object, which should have been initialized correctly in Device::CreateTexture or BaseTexture::ApplyStateChanges. The reason the current code is bad is two-fold: 1) unneeded state changes on a performance critical path, as texture binding is very frequent and 2) changing the state of a _previous_ texture object, not the current one. The next time we go to use that texture object we will have to reapply all of it's state again, another waste of time. I spent a lot of the weekend debugging wined3d (trying to get anisotropic working in World of Warcraft) and just commenting out the block of code greatly cut down on d3d trace messages and makes the code easier to understand (and faster).
2) device.c::SetupTextureStates() Similar to my previous remark, there seems to be an incorrect state change here as well. glTexEnvf(TEXTURE_FILTER_CONTROL, LOAD_BIAS), glBlendColor() and then glTexEnvfv(TEXTURE_ENV, TEXTURE_ENV_COLOR) are called. All of these are called for each texture state update. However these are all global render states, not per-texture contrary to the comment in the code. Since these states are already being set in Device::SetRenderState() I'm pretty sure they don't need to be set here as well.
Well that's all for now. Having started to really dig into wined3d, it seems that some effort in cleaning it up (coding style is way off) and stablizing it before it gets much more unwieldy is due. I'm also willing to help with such a task if there is a consensus that it should be done.
Regards, Aric
--- Aric Cyr Aric.Cyr@gmail.com wrote:
I just have a couple wined3d code questions/comments that I'd like to clear up and possible submit patches if my understanding is correct. If I am misunderstanding the code please correct me.
- basetexture.c:BindTexture() Just _before_ we call glBindTexture we call glTexParameteri and either set
GL_TEXTURE_MAX_LEVEL or GL_TEXTURE_MIN_FILTER. The comment in the code is:
"Always need to reset the number of mipmap levels when rebinding as it is a property of the active texture unit, and another texture may have set it to a different value"
However, this doesn't make any sense to me and I believe it to be incorrect. glTexParameter affects the currently bound texture object. If the current and next texture to be bound have different mipmaps levels or parameters it should not be necessary to change these settings as all the state is encapsulated correctly in the OpenGL texture object, which should have been initialized correctly in Device::CreateTexture or BaseTexture::ApplyStateChanges. The reason the current code is bad is two-fold: 1) unneeded state changes on a performance critical path, as texture binding is very frequent and 2) changing the state of a _previous_ texture object, not the current one. The next time we go to use that texture object we will have to reapply all of it's state again, another waste of time. I spent a lot of the weekend debugging wined3d (trying to get anisotropic working in World of Warcraft) and just commenting out the block of code greatly cut down on d3d trace messages and makes the code easier to understand (and faster).
Your right the mipmap level should only ever need to be set once, just after the texture is created. The problem is that this doesn't seem to work, at least with ATIs drivers this demo (http://www.codesampler.com/dx9src/dx9src_3.htm#dx9_texture_filtering) fails, along with many games and demos without some weird code. This is one of the two regression problems that are preventing d3d8-wined3d wrapper and evict managed resources from being checked in.
- device.c::SetupTextureStates() Similar to my previous remark, there seems to be an incorrect state change
here as well. glTexEnvf(TEXTURE_FILTER_CONTROL, LOAD_BIAS), glBlendColor() and then glTexEnvfv(TEXTURE_ENV, TEXTURE_ENV_COLOR) are called. All of these are called for each texture state update. However these are all global render states, not per-texture contrary to the comment in the code. Since these states are already being set in Device::SetRenderState() I'm pretty sure they don't need to be set here as well.
Again, I'm fairly sure that there are some things that don't work without reapplying the states all the time. The last time I looked at this was just after the last big patch I put out so I can't quite remember which demos/games didn't work.
Well that's all for now. Having started to really dig into wined3d, it seems that some effort in cleaning it up (coding style is way off) and stablizing it before it gets much more unwieldy is due. I'm also willing to help with such a task if there is a consensus that it should be done.
I'm just about to start tidying up directx.c, or at least the supported formats bit of directx.c and then I was going to tidyup drawprim a bit. What is the expected coding style, I just coppied that one that was being used.
Thanks, Oliver.
Regards, Aric
___________________________________________________________ Yahoo! Photos NEW, now offering a quality print service from just 7p a photo http://uk.photos.yahoo.com
Oliver Stieber <oliver_stieber <at> yahoo.co.uk> writes:
Your right the mipmap level should only ever need to be set once, just after the texture is created. The problem is that this doesn't seem to work, at least with ATIs drivers this demo (http://www.codesampler.com/dx9src/dx9src_3.htm#dx9_texture_filtering) fails, along with many games and demos without some weird code. This is one of the two regression problems that are preventing d3d8-wined3d wrapper and evict managed resources from being checked in.
I'd say check it in anyways... if you are worried about breakage on ATI machines, you could always check the gl_vendor and skip the code that causes problems on ATI machines.
Again, I'm fairly sure that there are some things that don't work without reapplying the states all the time. The last time I looked at this was just after the last big patch I put out so I can't quite remember which demos/games didn't work.
I'll test these on my ATI machines and check to see if these work-arounds are still valid/needed. If there are any bugs or issues with ATI's driver, please let me know the details and I'll communicate them to ATI.
I'm just about to start tidying up directx.c, or at least the supported formats bit of directx.c and then I was going to tidyup drawprim a bit. What is the expected coding style, I just coppied that one that was being used.
That would be great. If there is anything you'd like to delegate to me just let me know... maybe I'll start with a clean up of device.c. The only thing that really bothers me, style-wise, is the line length of some of the code. But wine doesn't seem to really have any style guidelines so I guess I'll have to live with this for now.
Regards, Aric
-
Aric Cyr -
Oliver Stieber