Advice on implementation of a fix for Incorrect merging of texture data. #1047

Compiling or Editing the Panda Source
1

I now understand what needs to change to fix this issue. The LookupKey for the _textures map in texturePool.h needs to contain more information. Right now it just keys off the file path where the texture is described, which leads to the bug. From discussion with @rdb the parameters that need to be followed are:

  1. format
  2. compression
  3. quality
  4. wrap mode
  5. minfilter
  6. magfilter
  7. and other things recorded in a sampler state

The first three I can add to loaderOptions that gets passed to Texture *TexturePool::ns_load_texture and then put those in the LookupKey. The others can all be encapsulated in a SamplerState object. My question is, how do I create this object? Since the object needs to survive beyond the scope of the loader (eggLoader) and becomes part of the map object _textures in TexturePool, I sense I need to allocate this memory and pass a pointer to the TexturePool. The TexturePool would then be responsible for deallocating that memory if that texture is removed from the _textures map object. Deallocation would happen in ns_release_texture and ns_release_all_textures. This would happen like so:

void TexturePool::
ns_release_texture(Texture *tex) {
  MutexHolder holder(_lock);

  Textures::iterator ti;
  for (ti = _textures.begin(); ti != _textures.end(); ++ti) {
    if (tex == (*ti).second) {
      LookupKey key = (*ti).first;
      delete key._texture_sampler;
      _textures.erase(ti);
      tex->_texture_pool_key = string();
      break;
    }
  }

One other thing, I noticed that apply_texture_attributes creates the sampler object that I want. However, this is called after the load. I am assuming that other texture attributes get populated during this load, so I do not want to move the apply_texture_attributes call to earlier in the function. Is that correct?

Let me know if this is not the best way to do this. Thanks

2

We can just store the SamplerState by-value on the LookupKey and on the LoaderOptions. There’s no need to allocate memory for it on the heap, I think.

I think we want to refactor EggLoader to pass the texture attributes into the loader via the LoaderOptions.

3

Ah, so store a value in the LookupKey, not a pointer. Then as it gets passed in the LoaderOptions a copy is made by the assignment. So as long as the SamplerState is declared in the LookupKey, the memory will be there for it. Ok, I will make that switch.

I saw my pull request broke the build because I needed to include EggTexture.h in LoaderOptions.h. I will have to look at that.

4

After looking at this a bit, I don’t know if this will work. Right now, it is not possible to declare a SamplerState object as part of LookupKey. If you want to do that, you have to include samplerState.h. This then creates a circular reference between samplerState.h, which includes bamReader.h which includes loaderOptions.h, that is where you put the samplerState.h include. Using a pointer to SamplerState in LookupKey and using a forward declaration can get around this. Thoughts?

5

Ah, interesting. However, I think we could get rid of the bamReader.h include in samplerState.h and replace it with a forward declaration, since the latter only uses BamReader by pointer, in a method declaration.

6

Ok. That will work. I will try to work on this tomorrow. I am excited to have a solution for this.

1 Like
7

Hmm, how do I initialize a default sampler state object? This has to happen in the LoaderOptions constructors? Do I use something like this?
_texture_sampler(SamplerState())

8

You don’t have to initialise it, because it has a default constructor.