Shadersystem / Texturescache

[greebo]

Ok, I thought a bit about how it could be possible to implement the caching functionality into the Shadersystem and this is what I came up with:

 

 

Basically it looks similar to what is implemented right now (I think), with these changes:

 

- The Texture object (formerly qtexture_t) itself does not keep a reference to its Constructor anymore.

- There is a distinction between simple and complex Textures. A simple texture is constructed from a single file without further "treatment". These can easily be cached (by using its filename as key). A complex texture is constructed using addnormals() and such. These are unlikely to be used more than once and are considered "uncacheable", therefore they are given a unique key (like $customBump1 or anything else with a unique index number).

- The GLTextureManager maps between the the keys and the Texture objects. The filename keys are easily catched by the texture manager, therefore it refuses to create a new OpenGl Binding and returns the TexturePtr to the existing texture instead. Internal reference counters are maintained to unload unused textures from memory.

- Everything denoted with "Ptr" as suffix represents a boost::shared_ptr of that type.

 

Any suggestions for improvements / simplifications?

[OrbWeaver]

Ok, I thought a bit about how it could be possible to implement the caching functionality into the Shadersystem and this is what I came up with:

 

Nice diagram, that represents pretty much how I understand things as they should be, and I don't see any major problems architecture-wise.

 

- The Texture object (formerly qtexture_t) itself does not keep a reference to its Constructor anymore.

 

OK, I did not realise it was doing this anyway, but it certainly doesn't seem necessary.

 

- There is a distinction between simple and complex Textures. A simple texture is constructed from a single file without further "treatment". These can easily be cached (by using its filename as key). A complex texture is constructed using addnormals() and such. These are unlikely to be used more than once and are considered "uncacheable", therefore they are given a unique key (like $customBump1 or anything else with a unique index number).

 

I don't agree with this distinction. Complex textures are needed more than once -- they are needed every time the Shader is rendered, just like with ordinary textures. If you shader uses addnormals(), you certainly don't want to be recalculating the combined image every frame.

 

You are correct that they do need a fabricated name however, because they don't map directly onto texture files. Something like "_shadername_diffuse" ought to be fine, since it shouldn't be used as a normal texture name but is easily constructed from the shader.

 

Other than the name, both simple and complex textures should be identical -- they are both encapsulated in a TextureConstructor, which is provided along with the name (for caching purposes) to the GLTextureManager to get a GL binding.

 

- The GLTextureManager maps between the the keys and the Texture objects. The filename keys are easily catched by the texture manager, therefore it refuses to create a new OpenGl Binding and returns the TexturePtr to the existing texture instead. Internal reference counters are maintained to unload unused textures from memory.

 

Yes, but without internal reference counters if possible. I think you could use something like:

 

std::map<std::string, boost::weak_ptr<Texture> >

 

in your cache, and whenever a TextureConstructor is provided to get a binding, you

 

1. Check in the map for a weak pointer. If not found, skip to (4)

2. Test the weak pointer to see whether it is valid (I think you try to convert it to a shared_ptr and if the result is empty, this means the object was deleted. Boost has docs).

3. If the weak pointer is valid, return the pointed result as a shared ptr, otherwise...

4. ...evaluate the TextureConstructor to get a TExturePtr (strong shared_ptr), create a weak_ptr from it and put it into the cache, then return the shared_ptr.

 

You can see from this that the cache does not "own" the pointed objects or increment their reference counts, it just points to them and allows you to check if they have been deleted by the normal shared_ptr usage.

 

Any suggestions for improvements / simplifications?

 

Other than the comments above, it's looking good. It's amazing what difference using the correct names makes - call it GLTextureManager and it's obvious what it does, call it TextureCache and there is confusion over its purpose and how it should be designed.

[OrbWeaver]

I notice that you have assigned the map expressions feature -- probably best if I take that, because I know of some issues with the current code which need to be fixed first (I plan to get rid of the enum-based approach and use interface + derived classes to represent the parse tree). Once the shader/texture design is done, it should be easy to create TextureConstructors from the map expressions using the new system.

[greebo]

I don't agree with this distinction. Complex textures are needed more than once -- they are needed every time the Shader is rendered, just like with ordinary textures. If you shader uses addnormals(), you certainly don't want to be recalculating the combined image every frame.

Sorry, I should have made this clearer, with more than once, I meant that this specific image is being used by more than one shader

 

Imagine two stone textures that base on the same diffuse map, but with different bumpmaps, the diffuse map should not be stored twice in the graphics memory. "Complex" or "calculated" textures are unlikely to be shared across shaders.

 

Yes, but without internal reference counters if possible. I think you could use something like:

 

std::map<:string boost::weak_ptr> >

 

in your cache, and whenever a TextureConstructor is provided to get a binding, you

 

1. Check in the map for a weak pointer. If not found, skip to (4)

2. Test the weak pointer to see whether it is valid (I think you try to convert it to a shared_ptr and if the result is empty, this means the object was deleted. Boost has docs).

3. If the weak pointer is valid, return the pointed result as a shared ptr, otherwise...

4. ...evaluate the TextureConstructor to get a TExturePtr (strong shared_ptr), create a weak_ptr from it and put it into the cache, then return the shared_ptr.

 

You can see from this that the cache does not "own" the pointed objects or increment their reference counts, it just points to them and allows you to check if they have been deleted by the normal shared_ptr usage.

Ah, I think I need some clarification here.

 

I first assumed that the ImageConstructors themselves communicate with the GLTextureManager and register their images (push the information), but it might be wiser to let the GLTextureManager control it (pull), when the actual openGL binds are made. This would require, that the Manager doesn't map to the Texture objects (as they don't contain any information about how to construct them), but to the TextureConstructor instead.

 

I will update the diagram and then we'll see if I get it right.

 

[greebo]

I notice that you have assigned the map expressions feature -- probably best if I take that, because I know of some issues with the current code which need to be fixed first (I plan to get rid of the enum-based approach and use interface + derived classes to represent the parse tree). Once the shader/texture design is done, it should be easy to create TextureConstructors from the map expressions using the new system.

I didn't mean to change the Parser, but preparing the shader module to use the system as proposed in the diagram (i.e. the GLTextureManager and such), but I've got no objections if you want to take over this task.

[OrbWeaver]

Sorry, I should have made this clearer, with more than once, I meant that this specific image is being used by more than one shader

Imagine two stone textures that base on the same diffuse map, but with different bumpmaps, the diffuse map should not be stored twice in the graphics memory. "Complex" or "calculated" textures are unlikely to be shared across shaders.

 

OK, agreed. A complex texture is not needed from another shader, thus it is only necessary that the OWNING shader knows its cache name, not anything else.

 

Ah, I think I need some clarification here.

I first assumed that the ImageConstructors themselves communicate with the GLTextureManager and register their images (push the information), but it might be wiser to let the GLTextureManager control it (pull), when the actual openGL binds are made. This would require, that the Manager doesn't map to the Texture objects (as they don't contain any information about how to construct them), but to the TextureConstructor instead.

I will update the diagram and then we'll see if I get it right.

 

You were right the first time (although I do prefer the new diagram font-wise).

 

An ImageConstructor doesn't register anything -- it is simply a "lazy evaluation" of an image. Rather than saying to the GLTextureManager "Here is an Image, get me the texture binding for it", you say "Here is the name of the texture binding I want. If you don't have it, here is an object you can use to construct it and add it to your cache for future lookups".

 

You could avoid using ImageConstructors entirely, but this would be less efficient because you would either have to calculate and provide the Image to the GLTextureManager in case it couldn't find what you were looking for (expensive) rather than just an object that constructs the image if necessary (cheap), OR you would have to use a two-stage process where you ask the GLTextureManager for a binding, it throws an exception or returns NULL because it doesn't have it, then you have to trap this, calculate the image and send it back to the GLTextureManager to create a binding from it. This second technique is workable and not that inefficient, but the lazy image constructors seem slightly more elegant and are closer to what we have already.

 

As regards to what the GLTextureManager needs to cache, I don't think it needs to retain the ImageConstructor at all, because this will be provided by the Shader object when it asks for a binding. All it needs to cache is a named table of weak pointers to Textures, which are either converted to strong pointers and returned (if the texture still exists), or replaced with a pointer to a new object created by calling the ImageConstructor.

 

I didn't mean to change the Parser, but preparing the shader module to use the system as proposed in the diagram (i.e. the GLTextureManager and such), but I've got no objections if you want to take over this task.

 

Presumably you will have to adjust the shader module anyway to avoid it breaking when the new system is implemented? The Map Expressions task refers specifically to implementing addnormals() and the like, which are currently detected but don't actually do anything.

[greebo]

As regards to what the GLTextureManager needs to cache, I don't think it needs to retain the ImageConstructor at all, because this will be provided by the Shader object when it asks for a binding. All it needs to cache is a named table of weak pointers to Textures, which are either converted to strong pointers and returned (if the texture still exists), or replaced with a pointer to a new object created by calling the ImageConstructor.

If the GLTextureManager just has the name and the TextureWeakPtr, how would it know which ImageConstructor to call? The Texture doesn't contain this information, this is stored within the Shader object (according to my diagram, although we could add such an ImageConstructorPtr to the Texture class).

 

Therefore I would have thought that it is more important for the GLTextureManager to know the ImageConstructorPtrs instead of the TextureWeakPtrs (which can be obtained through the former).

 

Presumably you will have to adjust the shader module anyway to avoid it breaking when the new system is implemented? The Map Expressions task refers specifically to implementing addnormals() and the like, which are currently detected but don't actually do anything.

Er, yes? (I don't understand entirely) Do you rather want to do this yourself or should I try to do it?

 

I believe that this will require two or three iterations till the system is working without glitches. We also need to consider the order the methods are called in (seems obvious, but I think there might some culprits).

[OrbWeaver]

Sorry, I should have made this clearer, with more than once, I meant that this specific image is being used by more than one shader

Imagine two stone textures that base on the same diffuse map, but with different bumpmaps, the diffuse map should not be stored twice in the graphics memory. "Complex" or "calculated" textures are unlikely to be shared across shaders.

 

I now understand what you mean here -- the whole point of the GLTextureManager caching textures by name is because a texture might be used by more that one shader. The Shader object will retain the qtexture_t (Texture) object associated with all of its textures, so in the case of complex textures which are NOT used by more than one shader, it is not necessary for the GLTextureManager to cache them (although it shouldn't really matter, unless memory becomes a problem).

[OrbWeaver]

If the GLTextureManager just has the name and the TextureWeakPtr, how would it know which ImageConstructor to call? The Texture doesn't contain this information, this is stored within the Shader object (according to my diagram, although we could add such an ImageConstructorPtr to the Texture class).

 

Well it could cache both. There are really two possibilities:

 

1. During parsing of MTRs, the ShaderSystem creates ImageConstructors as necessary from map expressions, and sends them to the GLTextureManager for registration along with either the name of the texture file (simple textures) or a generated name based on the shader (complex textures). When the GL texture binding is later required, the CShader requests it by name from the GLTextureManager, which either finds it in its weak pointer cache, OR calls the saved ImageConstructor to produce it.

 

or

 

2. The ShaderSystem does not pass ImageConstructors to the GLTextureManager during parsing, but retains them internally on the CShader object. When the CSHader is asked for its textures by the renderer (with getDiffuse or whatever), it checks to see if it has a texture binding already, and if it doesn't it passes the respective ImageConstructor to the GLTextureManager there and then, to get a binding. This is more similar to how I think it works currently with your changes, and what I thought your first diagram was representing (although I missed the part about ImageConstructor registering textures itself).

 

It's really a question of where the ImageConstructors are stored -- in the GLTextureManager, or on the CShader object which supplies them at the appropriate time. Neither of these options jumps out at me as being significantly better than the other, at this point, so maybe it doesn't really matter.

 

Er, yes? (I don't understand entirely) Do you rather want to do this yourself or should I try to do it?

 

I suggest changing what you need to keep things working, but I wouldn't worry for now about adding new functionality to create combined normal maps or other image processing, unless you particularly want to.

 

I believe that this will require two or three iterations till the system is working without glitches. We also need to consider the order the methods are called in (seems obvious, but I think there might some culprits).

 

Indeed, that is likely. Generally I think that construct-on-demand is a good strategy for avoiding ordering issues (rather than construct-in-advance, which requires knowledge about what is going to happen in future and at what time), although it is probably not possible to eliminate all issues regarding order.

[greebo]

I prefer this method, because then the knowledge of how to create a texture remains in the shader object.

2. The ShaderSystem does not pass ImageConstructors to the GLTextureManager during parsing, but retains them internally on the CShader object. When the CSHader is asked for its textures by the renderer (with getDiffuse or whatever), it checks to see if it has a texture binding already, and if it doesn't it passes the respective ImageConstructor to the GLTextureManager there and then, to get a binding. This is more similar to how I think it works currently with your changes, and what I thought your first diagram was representing (although I missed the part about ImageConstructor registering textures itself).

 

I added some more comments to this diagram:

The central objects seems to be the Shader object, which handles an incoming capture() request by trying to deliver a strong TexturePtr.

If the TexturePtr is still empty, the Texture is not yet instantiated and it invokes its local ImageConstructor, which in turn manages all the technical stuff like calculations, file load requests and so on.

 

I'm not sure if I need weak pointers, because the only object that needs to check if the textures are instantiated is the shader object itself, and it owns the actual strong pointers itself. Given the case a specific texture is shared by multiple shaders - well, that's the perfect excuse for a boost::shared_ptr, isn't it?

 

However, I'm sure that in the end of the day we'll have a decent system up and running, so I'd better get to work.

 

I suggest changing what you need to keep things working, but I wouldn't worry for now about adding new functionality to create combined normal maps or other image processing, unless you particularly want to.

Indeed, that is likely. Generally I think that construct-on-demand is a good strategy for avoiding ordering issues (rather than construct-in-advance, which requires knowledge about what is going to happen in future and at what time), although it is probably not possible to eliminate all issues regarding order.

I will have a go at that (and most likely run into problems, then we'll meet again here )

[OrbWeaver]

I prefer this method, because then the knowledge of how to create a texture remains in the shader object.

 

I also have a very slight preference for this method, although not enough to definitively state it is the "right" way to do it. Glad we agree on this point though.

 

The central objects seems to be the Shader object, which handles an incoming capture() request by trying to deliver a strong TexturePtr.

 

That would be a getDiffuse() (etc) request -- IShader doesn't define a capture() method. All of the getDiffuse()-like methods should return a TexturePtr, yes.

 

If the TexturePtr is still empty, the Texture is not yet instantiated and it invokes its local ImageConstructor, which in turn manages all the technical stuff like calculations, file load requests and so on.

 

Almost. The CShader does not invoke the ImageConstructor directly, but passes it to the GLTextureManager to get a binding from it. If it is a simple texture and has already been bound by another shader, the GLTextureManager just returns the existing cached TexturePtr, otherwise the GLTExtureManager invokves the ImageConstructor, binds the result and returns the new TexturePtr.

 

I imagine that the GLTextureManager will be something like this:

 

class GLTextureManager {

  // Cache
  std::map<std::string, TexturePtr> _cache;

public:

  // Get a binding for the named texture, using the given loader to create it if not found in cache
  TexturePtr getBindingFor(const std::string& name, ImageLoaderPtr loader) {

  if ( // name is found in cache
  {
	 return _cache[name];
  }
  else {
			  Image newImg = loader->loadImage();
			   // bind to OpenGL newImg. and create a new TexturePtr with its details
	   _cache.add(name, newTexturePtr);
	   return _cache[name];
   }
  }
};

 

I'm not sure if I need weak pointers, because the only object that needs to check if the textures are instantiated is the shader object itself, and it owns the actual strong pointers itself. Given the case a specific texture is shared by multiple shaders - well, that's the perfect excuse for a boost::shared_ptr, isn't it?

 

The only purpose of weak pointers is so that Texture objects can drop out of the GLTextureManager's cache automatically, without needing explicit reference counting. This may not be necessary, if we don't mind Textures remaining in the GLTextureManager indefinitely (which we probably don't, I'm not sure if there is a problem here or not).

 

It might be less confusing to simply forget about weak pointers for the time being; they can be introduced later if necessary.

[greebo]

Ok, I think I can work with that one, I updated the diagram yet again (mostly for myself so that I don't forget the relationships )

 

[OrbWeaver]

YES. The diagram is now pretty much a 100% match for my own internal concept. Just a minor nit: it should be ShaderLibrary::capture() not Shader::capture() up at the top.

 

Could you put this image on the Wiki? It would be good to have it alongside any documentation on these components.

[greebo]

Aye, sir! Will put this on the Wiki as the system is growing, because it may change during development.

 

[greebo]

Time to report what I've done so far:

• I implemented the GLTextureManager and the ShaderLibrary as indicated by the diagram. The CShader object realises/instantiates its textures as soon as they are demanded by the Rendering system by using the TextureConstructors.
  
• I replaced all qtexture_t* with TexturePtr (boost::shared_ptr of course).
  
• All IShader* pointers have been converted to IShaderPtr.
  
• The Textures are unloaded from OpenGL in the destructor of the Texture structure now, which makes more sense IMO.
  
• The content of the file shaders.cpp has been refactored into the Doom3ShaderSystem (which owns the ShaderLibrary and the GLTextureManager, btw.) or into the ShaderFileLoader class (parseShaderDecl). The file shaders.cpp is gone.
  
• The internal reference counting of Textures is gone and the refcounting of CShaders is deactivated. This is de facto done by the boost::shared_ptr class now (by calling unique() or use_count()).
  
• Shader textures are not removed and reloaded from/into graphics memory when switching render modes >> major performance boost, see below.
  

What's missing:

• The TextureConstructors are very basic, no MapExpression evaluation yet (this is another task).
  
• The Image Post-Processing is only partly implemented (I'm working on that one right now).
  
• The TexturesCache is still there (and soon to be removed, when I'm done with the latter point).
  

Regarding the Image Post-Processing:

The "old" system realised and unrealised every single texture when switching between rendering modes, which is the main reason for the huge delay. Not only the shader system reloaded every texture from disk, the image post-processing was performed in each step, which is insane IMO. This is resolved now, but I still have to do some tests for possible "texture leaks".

 

The image processing itself was definitely not optimised in the old system. The gamma for each texture was applied even at gamma values of 1.0 (which means no change), which basically meant that every single pixel was taken and re-saved into the pixel storage without any change (of course this was done before a possible texture downsize). Now multiply this by the number of textures in bonehoard or mansion_alpha and you know what to expect.

 

Another thing was the calculation of the flatshade colour, which basically takes the mean value of the RGB channel of the image. The old system took every single pixel in account, which is totally unnecessary - I changed it to use every 20th pixel (which could still be reduced, but I didn't perform any test-runs) and it yields good results.

 

So far, so good.

[OrbWeaver]

Sounds very good so far. Just a couple of queries.

 

[*]The internal reference counting of Textures is gone and the refcounting of CShaders is deactivated. This is de facto done by the boost::shared_ptr class now (by calling unique() or use_count()).

 

You shouldn't normally need to check use counts yourself, these are intended for debugging (in particular, use_count() can be inefficient). The idea of shared_ptr is that it totally takes away the responsibility for reference counting, so you don't need to do this yourself. Is the idea that objects will fall out of the cache when nothing else is using them (which will ensure unique() is true)?

 

The image processing itself was definitely not optimised in the old system. The gamma for each texture was applied even at gamma values of 1.0 (which means no change), which basically meant that every single pixel was taken and re-saved into the pixel storage without any change (of course this was done before a possible texture downsize). Now multiply this by the number of textures in bonehoard or mansion_alpha and you know what to expect.

 

These improvements sound very promising. I assume they only improve the speed when switching between render modes, not the rendering itself (when moving the camera)?

 

Another thing was the calculation of the flatshade colour, which basically takes the mean value of the RGB channel of the image. The old system took every single pixel in account, which is totally unnecessary - I changed it to use every 20th pixel (which could still be reduced, but I didn't perform any test-runs) and it yields good results.

 

What happens if the image is smaller than 20 pixels (i.e. 16x16)? It might be better to take a few samples based on the size of the image, say at U=0.0, 0.5 and 1.0 with V=0.0, 0.5 and 1.0.

[greebo]

Is the idea that objects will fall out of the cache when nothing else is using them (which will ensure unique() is true)?

Exactly. The TexturePtrs are stored in a std::map, therefore the use_count is never reaching 0. If a shader is destroyed, it calls the checkBindings() method, which throws out the unique()==true TexturePtrs, which in turn triggers the destructor.

These improvements sound very promising. I assume they only improve the speed when switching between render modes, not the rendering itself (when moving the camera)?

True, the rendering itself is not affected by this, but the switching. After I'm through with the changes the very first switch between rendering modes will still take some time, but afterwards it can almost happen at no cost.

What happens if the image is smaller than 20 pixels (i.e. 16x16)? It might be better to take a few samples based on the size of the image, say at U=0.0, 0.5 and 1.0 with V=0.0, 0.5 and 1.0.

A 16x16 image has 256 pixels in total, and I step over them with a 20 pixel increment. I already had that thought when choosing the increment. And I figured that there wouldn't be any 4x4 images. Yet, if there were any of them, it wouldn't do any harm, as the flatShadedColour will take the colour of the very first pixel.

[greebo]

I'm finished with the image post-processing, this is what I've changed:

 

The texture gamma is applied after the texture has been scaled to reduce the amount of pixels to be scanned (won't have much impact, but it's a start).

 

I'm pretty sure that the GtkRadiant mip map creation code was broken (it appeared to pass the wrong dimensions, so it ended up having a full-sized texture and a single 1x1 mip, but nothing in between). I changed this to use the gluBuild2DMipmaps command that lets the driver/hardware take care of the calculations. This may hopefully reduce the delay when loading the textures and may even have some influence on rendering speed.

 

Next step is to remove the old TexturesCache module, as it's not needed anymore.

[greebo]

Okay, the TexturesCache is gone now. All the important functionality has been moved into the Shaders module, including a method to load a texture from disk as needed by the Overlay module.

 

A bit of tidying remains to do in the CShaders class, but this should be all that's left.

[New Horizon]

Wonderful work!

[OrbWeaver]

I have merged in my temporary branch for working on the crash when opening a .bak file, which includes a replacement of the ReferenceCache's used of HashedCache with a std::map containing weak pointers, if you want an example of such an implementation.

 

This was actually necessary, because if the std::map contains shared pointers and then is cleared, the pointed objects will be destroyed. In the case of the ReferenceCache this was bad news because the destructors called a long chain of functions which eventually resulted in a call back to capture(), meaning that it was trying to insert into a map which was simultaneously being cleared (crash time). Using weak pointers means that clearing the map does not destroy any objects, and this re-entrant condition is not triggered.

[greebo]

Ah, interesting. For me the question arises why the system is designed in such a way that the destruction of an object triggers its re-insertion, but I don't know any of the implementation details of course, so it may really be necessary. Sounds weird nonetheless.

 

This means that there is only one HashedCache left, in renderstate.cpp, if I recall correctly.

[OrbWeaver]

Ah, interesting. For me the question arises why the system is designed in such a way that the destruction of an object triggers its re-insertion, but I don't know any of the implementation details of course, so it may really be necessary. Sounds weird nonetheless.

 

Because it's bloody stupid, that's why. The chain of events goes something like: clear cache -> delete ModelResource object -> set model to null -> some reference counting stuff with NodeSmartReferences -> trigger model changed callbacks on certain entities -> set model to "" -> capture "" in the Reference Cache -> crash.

 

This means that there is only one HashedCache left, in renderstate.cpp, if I recall correctly.

 

I have done some refactoring of renderstate already, so I may well deal with that in due course.

[greebo]

I've had a look at the recent issue on the Bugtracker (Reload Surface) and realised that we'll need some boost::weak_ptrs here (I think you already suggested this once, IIRC).

 

Reason is, currently there is the ShaderLibrary which holds the mapping between names and ShaderPtr (std::map<:string shaderptr>) on the one side, and the OpenGLShader implementation, which holds another shared_ptr, hence the reference count is never reaching zero, hence no texture reload, even if I clear the ShaderLibrary.

 

I wonder what would be best? Which class should hold the only strong ptr? I assume this is the ShaderLibrary, so that a ShaderLibrary::clear() call would destruct the shader objects. The weak_ptrs held by the OpenGLShader objects would become invalid then, which has to be caught and a realisation request has to be sent.

 

Am I thinking right here or should I do it the other way round?

[OrbWeaver]

Refresh my memory -- what is the ShaderLibrary doing? Is this the main source of Shaders, or is it just a cache? If it is a cache, then it should use weak pointers, yes.