greebo

I prefer this method, because then the knowledge of how to create a texture remains in the shader object. 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 will have a go at that (and most likely run into problems, then we'll meet again here )

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

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.

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

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?

(psst, thelvyn, why are you still posting in your application thread?)

I spent some time typing in a long reply to your post, which I discarded, because basically it boils down to ditching the TexturesCache, because it's uncapable of caching textures that consist of multiple images (constructed by addnormals() or consorts). Additional to the actual OpenGL binding routines it only provides methods for image post-processing that can as well be moved somewhere else (in fact they should be moved from a Cache, because that's not its competence). Nothing else apart from the Shader module is using the TexturesCache (save the Texture Window and the renderstate.cpp, but I believe that can be changed rather easily). I vote for incorporating the whole functionality (apart from the image pixel processing, gamma, etc.) into the shaders module. The only thing we have to consider is preventing the same pixel data being loaded into OpenGL twice. There may be some situations where the ImageConstructors produce the "same" image, but these cases could be hard to catch and are subject to optimisation in later iterations. We can still use the ImageConstructor class - depending on who is using it, we should change its interface to deliver a shared_ptr (or the renamed structure, I don't like that name) rather than an Image*. If there is post-processing happening before inserting them into OpenGL, we should stick with Image, otherwise it could as well deliver the qtexture_t structure.

Yes, that's the purpose of the TextureCache and its realise() and unrealise() methods, they load/unload the images into/from graphics memory (and the TexturesCache provides some image post-processing like gamma, texture rescaling and compression). Yes, the responsibilites have moved a bit, which I think is unavoidable if the image construction is to be abstracted and moved away from the TexturesCache. The shaders module is the only thing that knows how the actual image should be constructed. The actual low-level image loading is handled by the modules in plugins/image/, only ImageLoaderModuleRefs are held within the ImageConstructors. However, the way the ImageConstructors are implemented can be changed in the future and moved as well, but the actual loading calls have to be called by someone and the shader module is the most likely candidate for this, as it is the only module that gets in touch with raw filenames.

I'm removing the LoadImageCallback as we speak, you can watch it dying on SVN ... The HashedCache is already gone and has been replaced with a std::map. The reference counter has been moved into the qtextures_t structure itself, as it's the best place for it IMO. The only way to capture() textures left is to provide an ImageConstructorPtr (which is a boost::shared_ptr to an ImageConstructor object). These objects provide your suggested Image* construct() method that delivers the readily fabricated image. These ImageConstructors can be as sophisticated as required and are owned by the Shaders module at the moment. I already added two default ImageConstructors that cover the current needs (one for loading textures from the darkmod/textures repository (tga, dds, jpg) and one for loading images directly off the disk), but these can be extended or replaced in the future. These ImageConstructors hold the information that is needed to fabricate the image and make use of the ImageLoader modules, which are needed to load the actual pixel data.

Just let me know when you've got something to do (open a thread or PM me). Sorry to be off-topic.

@Springheel: While we're talking about updating the website: If you ever need a hand with keeping stuff up to date (apart from writing the actual news) let me know, I'd happily help you out. I thought of tasks like preparing images for the screenshots sections or keeping the team members list up to date, whatever.

Ok, I need to get this right: The Shadersystem parses the material files and creates an IShader object for all the shader definitions it may find. The IShader object provides methods like getTexture(), getBump() and getSpecular() to retrieve the texture description structure (that contains stuff like width, height and the openGL texture id). To allow the TexturesCache to realise a certain shader it must call some kind of load() function that delivers an Image* object with the readily fabricated pixel-data (so it can be stored in the graphics memory). I understand that this load() method must not contain any low-level information like filenames, because this should be the business of the shader system. So the filenames have to be stored somewhere, I guess this is the IShader class implementation? If I understand this correctly, the actual load-image-from-disk process has to be moved away from the TexturesCache into the Shadersystem, otherwise the filename abstraction would not work. Correct? (I agree that ImageLoader is a bit of a misnomer here, and should be renamed to ImageProducer or ImageConstructor.)

@Darkness_Falls: I honestly don't think Springheel was referring to you when talking about "armchair modding".

When I'm finished the LoadImageCallback& will be gone and replaced by an ImageLoader* pointer, so that the ImageLoader class can be used to load the image on demand. Is this what you meant?

I've chosen to work directly on the trunk, this way I don't have to cross-merge the changes. My first commit is already on SVN and implements a TGALoader class into the existing framework. As soon as the other loaders are existing as well, I'll start to change the TexturesCache itself. You can have a look at it if you want and tell me if I made some serious design mistake, although the current implementation is not the final thing (which will have the LoadImageCallback& and function pointers removed from iimage.h).

Do you want me to branch off with the ImageLoader changes or can I work in the trunk?

Ah, didn't know about the package size issue, doesn't make too much sense then to move stuff relying on GTK into the modules. The benefit of having 1) a cleaner interface and 2) a compile time decrease when moving stuff into the modules does still apply, doesn't it? Moving the Overlay module into the main codebase won't be much of a problem, as its code is quite isolated.

I looked at my problem again this morning and it really seems that the LoadImageCallback& somehow becomes invalid. In the original HashedCache, the thing is stored twice: once in the qtexture_t object and once in this cursed TextureKey structure (a std::pair of texture name and texture loader, both redundant (*) to the information stored in the qtexture_t). My map works without that key and DarkRadiant always crashes when using the LoadImageCallback from the qtexture_t object. General question: I'd like to change the LoadImageCallback to something else, that allows more validity-checking than the current system. What would you suggest? Apart from the Shaders module there is only this GlobalTexturesCache module that invokes the image loaders, so it would not be too much work to change their calls. Would it be sufficient to define an abstract base class ImageLoader which all the actual loaders derive from and a pointer to them would be passed? Or is using pointers not the solution of choice here? (*) I'm not entirely sure if this info is the same as it's a reference to a LoadImageCallback& which is hard to check if it's valid or not. If this information is not identical the whole system is screwed IMO. Why store an invalid pointer in the qtexture_t in the first place?

What about libboost-filesystem-dev? Do you have this package installed? I find it a bit unconvenient having to download every single boost package separately...

No need to apologise, I should have read the documentation (yes, this class has some lines of comments, unbelievable, but these are the facts). I learned it the hard way, but I still learned something. I already made an attempt to implement the referenced map (which was done rather quickly), and I think I got it right, but there must be a problem with the LoadImageCallbacks as well. Perhaps they are copied differently in the HashedCache environment than in the std::map things, but I really have no idea why the pointers to the load functions get corrupted. I will try my luck again tomorrow, as my motivation is too low currently.

Wow, this was a waste of time: I spent the last four hours trying desperately to replace this HashedCache thing in textures.cpp. It is not an ordinary std::map, it does maintain an internal reference counter for each of the elements as well. As soon as the reference counter reaches 0, the templated "destroy" method is called. I first tried to replace it with a std::map, which was of course a complete failure (crashes after crashes). Afterwards I added a reference counter as well, but it's still crashing as soon as I change the gamma value in the preferences (which triggers a re-realise call, which is good for testing). The point where it crashes is that damned LoadImageCallback method, where it invokes the loadImage() pointer, which somehow gets bargled up, and I can't say why as nothing is changing it. I might as well try to replace that structure too but that is something I want to avoid. Now, before I give up: is there a boost equivalent to such a std::map with an internal reference counter of its objects? As a last attempt I will try to replace the texture map with shared_ptrs, but I don't know exactly, if that works out.

The aspect ratio doesn't matter anyway, the texture coordinates already take care of that. At least it worked in my case (I hadn't to re-texture a single brush). I just wanted to state, if there is an odd dimension in any of my textures, I rather stretch the according size up, so that Doom doesn't have to downsize at all. I don't want my 768 pixels become 512, I'd rather have my texture at 1024 pixels containing interpolated data than let Doom discard a third of the information. (Don't want to quabble around here, as we're probably talking about the same, just wanted to make my point clear.)

When I converted some of my textures into DDS format I had to rescale some of them as well (being 1024 x 768 for some reason). I ended up stretching the non-power-of-two dimension to the next greater step (which made the texture 1024x1024). As Orbweaver said, I'd rather waste some disk space than have the overall dimensions shrunk down.

Isn't a fish-eye effect totally unrealistic for a spy-glass? You get these on wide-angle camera lenses and door-spies, but not for a telescope. This looks like you're hiding behind your flat's door looking who's standing outside before letting them in. I'd agree that a small amount of abberation is ok and adds character, but that is a little too much I think.

Yes, but don't expect the user.xml to total around 100 Bytes; as soon as you press OK in the Preference dialog all the settings get saved by calling set() and this means they are instantly written into the user tree. I could make one more step and implement a comparison into the set() method that checks if the value to be written is any different from the default value (if that one even exists) and only writes changed values into the user tree. For this to work effectively it would mean that keyvalues matching the default values should be deleted from the usertree - but this could have some "Unforeseen Consequences™" (couldn't resist ) if the deleted node contained any subnodes. The workload for the registry would increase and I don't know if it's worth the hassle...