Jump to content
The Dark Mod Forums

Itches, Glitches & Anything Else...


Recommended Posts

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.

Link to comment
Share on other sites

Sorry, I should perhaps have been more clear when I said HashedCache was "just a std::map" -- as you have discovered it is not exactly the same, as it provides the caching/refcounting functionality you are grappling with. Although it can probably be replaced with some combination of std::map and shared_ptr, this may require some thought to implement correctly.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

Got a lot further this time...but now this...

 

/AutoSaver.o radiant/map/AutoSaver.cpp

radiant/map/AutoSaver.cpp:16:37: error: boost/filesystem/path.hpp: No such file or directory

radiant/map/AutoSaver.cpp:17:44: error: boost/filesystem/convenience.hpp: No suc h file or directory

radiant/map/AutoSaver.cpp:18:42: error: boost/filesystem/exception.hpp: No such file or directory

radiant/map/AutoSaver.cpp:34: error: ‘boost::filesystem’ has not been declared

radiant/map/AutoSaver.cpp:34: error: ‘path’ does not name a type

radiant/map/AutoSaver.cpp: In member function ‘void map::AutoMapSaver::saveSnaps hot()’:

radiant/map/AutoSaver.cpp:104: error: ‘Path’ was not declared in this scope

radiant/map/AutoSaver.cpp:104: error: expected `;' before ‘fullPath’

radiant/map/AutoSaver.cpp:107: error: ‘fullPath’ was not declared in this scope

radiant/map/AutoSaver.cpp: In member function ‘void map::AutoMapSaver::checkSave ()’:

radiant/map/AutoSaver.cpp:199: error: ‘boost::filesystem’ has not been declared

radiant/map/AutoSaver.cpp:199: error: expected type-specifier

radiant/map/AutoSaver.cpp:199: error: expected `)' before ‘f’

radiant/map/AutoSaver.cpp:199: error: expected `{' before ‘f’

radiant/map/AutoSaver.cpp:199: error: ‘f’ was not declared in this scope

radiant/map/AutoSaver.cpp:199: error: expected `;' before ‘)’ token

radiant/map/AutoSaver.cpp:228: error: ‘Path’ was not declared in this scope

radiant/map/AutoSaver.cpp:228: error: expected `;' before ‘autoSaveFilename’

radiant/map/AutoSaver.cpp:233: error: ‘autoSaveFilename’ was not declared in thi s scope

radiant/map/AutoSaver.cpp:238: error: ‘boost::filesystem’ has not been declared

radiant/map/AutoSaver.cpp:238: error: expected type-specifier

radiant/map/AutoSaver.cpp:238: error: expected `)' before ‘f’

radiant/map/AutoSaver.cpp:238: error: expected `{' before ‘f’

radiant/map/AutoSaver.cpp:238: error: ‘f’ was not declared in this scope

radiant/map/AutoSaver.cpp:238: error: expected `;' before ‘)’ token

scons: *** [build/debug/radiant/map/AutoSaver.o] Error 1

scons: building terminated because of errors.

Link to comment
Share on other sites

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?

Link to comment
Share on other sites

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?

 

Yes, that's exactly what I would suggest. Function pointers are evil, so replacing them with proper abstract interfaces and inheritance is a step in the right direction. Doing this would also facilitate the use of custom ImageLoader subclasses which encapsulate map expressions (such as addnormal), which can be created and inserted into the texture cache and which return the processed image when their loadImage function is called.

 

The TextureKey stuff is bloody daft, as you have noticed. Textures should be looked up by name, not by name + pointer-to-callback-function.

Link to comment
Share on other sites

Whatever you feel comfortable. The general rule is that the commits to the trunk should not break the build or regress important functionality, so you can either (1) make small, incremental changes to the trunk which don't break anything too much, (2) make large changes and commit them to a branch, merging back when complete, or (3) do everything locally and don't make any intermediate commits until the whole task is complete.

 

Note that you don't have to make a branch before you start working on it -- you can make a whole load of changes locally, then commit them to a new branch in a single step. This means that you could (if you wished) attempt (3) and then switch to (2) if it turned out you were halfway through a large incomplete task but didn't want to risk losing your work.

Link to comment
Share on other sites

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

Link to comment
Share on other sites

Your ImageLoader is different from what I was referring to above, as it seems to implement a service object which loads images of a given type from a provided file, whereas my ImageLoader was a replacement interface for the LoadImageCallback, that would represent a function object allowing delayed loading of a specific image.

 

I can't see any problem with the ImageLoader as implemented, but we may be talking at cross purposes regarding its purpose.

Link to comment
Share on other sites

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?

 

That is what I meant, but I don't think your current ImageLoader will meet all of the requirements, because

 

(1) It requires the filename of the image to be stored externally, presumably in the TexturesCache, and

(2) It won't support map expressions, because these may be made up of a number of images rather than a single one.

 

I was imagining that the interface would be something like

 

class ImageLoader
{
public:
  virtual Image* loadImage() = 0;
}

 

and the textures cache would store them in a map like

 

std::map<std::string, boost::shared_ptr<ImageLoader> >

 

Then, whenenever a texture is looked up, the textures cache just obtains the named image loader and calls its loadImage() function to construct the actual image. This means that the textures cache does not need to know any information about the image (filename, type etc), which can be created by the shaders module from a number of image maps when the particular ImageLoader's loadImage function is called.

 

This is at least how I understand the current implementation sort of works, it just does so with a whole load of ugliness involving void* and function pointers. It's a form of something called "lazy evaluation", whereby instead of storing the actual object (Image), you store a function object which can construct the required object on demand when called in a certain way. Perhaps the name ImageLoader is misleading, maybe it should be ImageProducer or something.

Link to comment
Share on other sites

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

Link to comment
Share on other sites

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

 

Correct.

 

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.

 

You may know more about this than I do, since I haven't examine the texture cache in a huge amount of detail, but my belief is that the texture cache does handle the filenames because textures are looked up only as a VFS path ("textures/blah/image1_local") which is then mapped, by the textures cache, onto the appropriate TGA or DDS file. However, rather than load the file directly it calls a LoadImageCallback functor which in turn loads the image itself - the purpose of which is to allow deferred processing of an image or set of images.

 

In most (all) cases it seems, the load image callback is actually just the QER_Load_Image function (or whatever it is called).

 

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.

 

There may be an argument for moving the image load processing into the ShaderSystem, indeed one might suggest that the SOLE purpose of the textures cache is handling the loading of texture files off disk and returning an Image containing their contents. It would then be up to the ShaderSystem to process this image as necessary (addnormals, rgb etc) and then bind it to OpenGL to get a unique texture number.

 

This is, however, a somewhat more involved redesign than simply replacing the function-pointer-based LoadImageCallback interface with one based on inheritance and pure virtual functions.

Link to comment
Share on other sites

I thought about this again last night, and I think that if the LoadImageCallback is only ever a pointer to the QER_Load_Image function, there is no need for it at all. You might be able to just remove it entirely, so that the texture cache just loads images on demand in the "obvious" way (you could use your original ImageLoader design for this), without the use of callbacks. I can then worry about upgrading the ShaderSystem to cope with generating processed images at a later date.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

 

Glad you've got rid of at least one instance of HashedCache. Internal (explicit) reference counting shouldn't normally be necessary however, if you use shared_ptr the reference counting is taken care of automatically. Is the idea that the cache itself will unload the objects once their reference count goes to zero?

 

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.

 

That sounds quite powerful, however I am slightly confused as to the changing roles of the TexturesCache and the ShaderSystem here. Previously it was

 

ShaderSystem: parse shaders and get the names of textures in the VFS, doesn't touch the actual texture files

TextureCache: look up VFS names, load the image files and return qtexture_t objects corresponding to them, including a bound OpenGL texture number

 

If the ShaderSystem now is responsible for loading the images off the disk and providing ImageConstructor objects to the TextureCache, what is the role of the TextureCache itself? Does it just bind the textures to OpenGL?

Link to comment
Share on other sites

Is the idea that the cache itself will unload the objects once their reference count goes to zero?

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

 

That sounds quite powerful, however I am slightly confused as to the changing roles of the TexturesCache and the ShaderSystem here. Previously it was

 

ShaderSystem: parse shaders and get the names of textures in the VFS, doesn't touch the actual texture files

TextureCache: look up VFS names, load the image files and return qtexture_t objects corresponding to them, including a bound OpenGL texture number

 

If the ShaderSystem now is responsible for loading the images off the disk and providing ImageConstructor objects to the TextureCache, what is the role of the TextureCache itself? Does it just bind the textures to OpenGL?

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.

Link to comment
Share on other sites

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.

 

I agree entirely, and this is where I was hazy before -- the ShaderSystem is the only module that knows about map expressions, but the loader callbacks were held in the TextureCache on individual images, rather than in the ShaderSystem.

 

Where I am still unsure, is exactly how this should be implemented. I think we have very slightly different views of how it should work, hence the confusion. One of the first things I have noticed in the code is this:

 

// Allocate a default ImageConstructor with this name
ImageConstructorPtr imageConstructor(new DefaultConstructor(displayTex));
m_pTexture = GlobalTexturesCache().capture(imageConstructor, displayTex);

 

What jumps out at me is that you are specifying the texture name twice, once in the construction of the ImageConstructor, and again in the call to capture(). This suggests that there is an unclear division of responsibilities between the ImageConstructor and the TexturesCache, both of which think they are responsible for selecting the texture filename. What would happen, for instance, if you specified a different filename in each case?

 

The solution I think you are implementing (correct me if I'm wrong) is:

 

1. ShaderSystem parses MTR file and determines the filename to load, and the processing that needs to be done.

2. ShaderSystem creates an ImageConstructor which encapsulates the loading and processing of that particular file or set of files (Question: can a loader be used to load more than one file? Presumably it can, because this is its main reason for existence?)

3. ShaderSystem passes both the filename and the ImageConstructor to the TextureCache to load. (Question: As above, what is the relationship between the filename passed here and the filename inside the ImageConstructor? What if they are different? If the ImageConstructor encapsulates more than one image, what filename is used here?)

4. TextureCache "evaluates" the ImageConstructor by calling its construct() method. It gets back an Image*, which it puts into the cache, binds to OpenGL and returns a qtexture_t object. This underlines a change in the responsibility of the TextureCache, previously it was responsible for loading texture files, now it does not load them itself but merely calls the ImageLoader supplied by the ShaderSystem and puts the result into the cache.

 

Steps (1) and (4) I am in broad agreement with, I just have some confusion over (2) and (3). I think part of the problem might be that you are using TextureCache::capture() for dual purposes -- finding and inserting. It would be clearer if you had two methods:

 

/* Capture the named texture, throwing an exception if it is not found. */
boost::shared_ptr<qtexture_t> capture(const std::string& name); // (*ahem*, raw pointers)

/* Insert a named texture into the cache, using the given Constructor to create it on demand */
[b]void insert(const std::string& name, ImageConstructorPtr img);

 

EDIT: Actually, that last clarification (with two functions) probably answers my questions about (2) and (3) in the list. Your TextureCache is just a cache, it does not load or deal with filenames in any way, but just stores named ImageConstructors, created by the ShaderSystem, for later evaluation. Maybe this means we don't need a TextureCache at all, it could just be handled internally by the ShaderSystem (unless there are other things which require access to the qtexture_t's themselves?)

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


  • Recent Status Updates

    • Petike the Taffer

      I've finally managed to log in to The Dark Mod Wiki. I'm back in the saddle and before the holidays start in full, I'll be adding a few new FM articles and doing other updates. Written in Stone is already done.
      · 4 replies
    • nbohr1more

      TDM 15th Anniversary Contest is now active! Please declare your participation: https://forums.thedarkmod.com/index.php?/topic/22413-the-dark-mod-15th-anniversary-contest-entry-thread/
       
      · 0 replies
    • JackFarmer

      @TheUnbeholden
      You cannot receive PMs. Could you please be so kind and check your mailbox if it is full (or maybe you switched off the function)?
      · 1 reply
    • OrbWeaver

      I like the new frob highlight but it would nice if it was less "flickery" while moving over objects (especially barred metal doors).
      · 4 replies
    • nbohr1more

      Please vote in the 15th Anniversary Contest Theme Poll
       
      · 0 replies
×
×
  • Create New...