moving images into units

[Darth Fool]

So in poking around the code to see how feasible adding a team color feature to unit graphics would be, (see http://www.wesnoth.org/forum/viewtopic. ... highlight=), I realized that although the algorithmic portion of the code was quite easy, it would be more difficult to do than I had originally hoped (probably pushing it to a post 1.0 feature). This is because as it stands right now, the unit_types image() function returns the name of the image file that should be used, not the actual image/SDL surface. So making my change requires going and changing the code everywhere a unit image is called. What I would propose doing is moving the functionality of getting the image into the unit clas. This still would require making changes to every-where the unit image is called, but at least the image processing could all be centralized and more easily maintainable.

Again, I realize that this is a pretty major change, and I am not trying to ask for advance appoval of this or a special exception to the feature cooling (especially without an actual patch done yet!), but I would like to know what the coders' opinion of this sort of change is and or hear(well, read) any suggestions on alternatives.

thanks,
DF

[Ayin]

What is wrong with returning the image path, instead of a surface? [1]. I mean, you can directly retrieve the (cached) surface from the image name.

[1] except that image::locator should be used instead, 'cause this makes the lookup much faster.

[Darth Fool]

The problem is that, for the functionality of processing the unit image based upon which side it is on, you need to recolor the unit everytime the image is called. If this is not done in a centralized way, then this function will need to be called in many places throughout the code, which leads to potential graphical glitches if someone goes and modifies things but doesn't add the code in the right spot. An alternative might be to use the existing system, but use a mangled file name for the id which includes the side information and push the processing down into the caching system. This would undoubtedly be computationally faster(but I doubt by that much(I could be wrong)), but you lose the current elegance of having the file name match the id. One could also generate files for each side, but that not only generates a lot of mostly redundant files, but it takes away the ability to have in game definition of side colors.

My plan would be to still use the caching system, just within a function in the unit class, not in a function that is called everywhere...

I am, of course, quite eager for someone to point out that I am being stupid in some way and that there is an elegant solution I have not thought of that will be easier/better for me to impliment...

[Ayin]

OK then, I understand now.

However: images were previously cached only by name; which would make your feature rather difficult to integrate with the current cache. However, the concept of "locator" was introduced, which extends the concept and also allows to cache some computed images.

To acheive your goal, the solution would be, IMO, to modify image::locator::value so that it can be used to also represent colorized images. Then, make it such as unit::image() returns the image::locator, and not the image string.

unit::image() is not called in that much places, so, this wouldn't be such a drastic change IMHO.

[Darth Fool]

Thanks for the feedback! It sounds like a good idea and I will stare at the code a bit and see if I think I can make it work.

[Dave]

Yeah, you definitely want to put the logic for retrieving and manipulating the images into the image module.

Among other things, there is an instance of the unit class for each unit. If there are ten Elvish Fighters on the same side, you don't want to have to have a set of images for each instance.

David