[wined3d] Code questions

[Oliver Stieber]

--- Aric Cyr <Aric.Cyr at 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.
> 
> 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).
> 
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.


> 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.

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