MapExpressions

[mohij]

I finished my exams. So I guess I could do some more coding ;-)

 

Are the MapExpressions finished for now? - Then I'd close the bug.

And what could I continue to work on then?

[greebo]

Sounds good! How was it going with the exams?

 

I'd say the MapExpressions are finished, so feel free to close the case.

 

The Image* to boost::shared_ptr transition comes into my mind as a next task. OrbWeaver, what would you suggest?

[mohij]

I guess I'll get 1.4 as the final score :-)

 

Closed the bug.

 

I think moving this to shared_ptr isn't that complicate, so I'll guess that'll do.

[OrbWeaver]

Yes, any replacement of raw pointers with smart pointers is a good step.

[greebo]

I just checked, the shaders and image modules are the only ones who're dealing with images, so this should be very painless. The only place where the actual images are allocated are the load() methods in the ImageLoader modules and the MapExpressions.

 

@mohij: While you're at it you could also check if the TextureConstructor base class is used by anything anymore - you can ditch that if it is unused.

[mohij]

Ditch means remove completely?

 

Edit: I just checked, and the FileLoader and DefaultLoader are still used.

[greebo]

Yup, if it's unneeded, there's no sense in keeping it.

[mohij]

Well the DefaultConstructor is used only once (in MapExpression.cpp). The FileLoader is used more widely.

So do you mean removing the baseclass and just using the Fileloader directly?

When moving MapExpression.cpp over to the FileLoader, then I could also ditch the DefaultConstructor.

[greebo]

Ah ok, seems like I was too quick on the draw - if it's still used then rather not remove it. I just didn't remember.

[mohij]

What is better,

Introducing a shared_ptr<Image> and another shared_ptr<RGBAImage> and using the ones I need, or

only introducing shared_ptr<Image> and cast to RGBAImage when needed?

[mohij]

btw. Should I open a new topic for this?

[greebo]

Feel free to open a new topic, if you feel it's worth it.

 

I think there is no common answer to your question, it depends on your code. Generally, it doesn't hurt to typedef the shared_ptrs right next to the class definition:

class Image
{
// blah
};
typedef boost::shared_ptr<Image> ImagePtr;

class RGBAImage
{
// blah
};
typedef boost::shared_ptr<RGBAImage> RGBAImagePtr;

So the shared_ptr is declared as soon as you include the respective class header.

 

During the allocation in the routines:

 

If you need to access methods from the RGBAImage interface it makes sense to allocate an RGBAImagePtr. This will be "compatible" to ImagePtr on return. Allocate it like this and return the up-casted shared_ptr on return:

   typedef boost::shared_ptr<RGBAImage> RGBAImagePtr;
  RGBAImagePtr newImg = RGBAImagePtr(new RGBAImage(width, height));

  newImg->blah(); // do some RGBAImage-specific stuff here

  return newImg; // upcasts implicitly to ImagePtr on return

 

If you just need to access methods from the Image interface, it's enough to allocate an RGBAImage, but don't store the RGBAImagePtr, like this:

   typedef boost::shared_ptr<Image> ImagePtr;

  // Allocate an RGBImage object, its pointer is downcast-able to Image*:
  ImagePtr newImg = ImagePtr(new RGBAImage(width, height));
  ...
  newImg->blah(); only Image-related methods are available here.
  ...
  return newImg;

 

edit: I edited this post a bit, so please don't get confused.

 

edit2: argh, confused upast with downcast.

[mohij]

I guess that's an exhausted answer, thanks.

[mohij]

Thinking a bit more about return values, I wonder whether it is better to return more general objects (upcast on return) or the specific object.

When returning the specific object won't have to be downcast, since you already have a child object and upcasting would happen from alone.

When returning base class objects, the user of the function will perhaps have to downcast (which is generally a bad idea) and in most cases I would have to upcast it inside the functions, so that would be double word.

 

So I'd say that returning objects as specific as possible is best. Perhaps I miss a point, so what do you think?

[OrbWeaver]

It depends on the context. If you are writing a method on the public interface, then the return value should be shared_ptr<Image>. If on the other hand you are developing an internal function for use within the module, it may be useful to return a more specific subclass to another part of the code.

 

There isn't really an answer along the lines of "always do X" -- it depends on what the problem is and what you are trying to achieve.

[greebo]

The ImageLoader modules should return an ImagePtr, which corresponds to the abstract Image base class. The RGBAImage class is "unknown" as far as the interface is concerned (I think the interface is declared in iimage.h). Whenever modules exchange data (in this case the shaders module and the image module) the base class pointers should be used.

 

Within a module you can choose whatever you want, as OrbWeaver pointed out.

[mohij]

I got the ImagePtr thing done yesterday, it compiles and even runs. But in dynamic lightning mode everything stays black.

Was there a bug that could have caused that? (so I don't hunt a bug that wasn't caused by my changes)

 

If you want to have a look:

imagePtr.zip

[greebo]

Are you synced with the latest revision on SVN? I recently fixed a bug causing the light falloff textures not being found, so that might be the reason. If you're synced, there is something else amiss.

 

I will check out your ZIP.

[mohij]

Heya, syncing fixed the bug. :-)

[greebo]

Your changes are committed, everything looked fine, well done.

 

I removed the obsolete release() method from the Image interface, so this migration should be finished now.

 

If you've got time for another task, this might be something for you: http://bugs.angua.at/view.php?id=319

[mohij]

Small programming question:

DefaultConstructor d;
if (_imgName == "_black")
d(GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_BLACK);
else
d = DefaultConstructor(_imgName);

return d.construct();

This code won't work. It could be done with pointers or a temporary variable, but is there an elegant way to do it? Can I declare a class and construct it later on?

[greebo]

Small programming question:

DefaultConstructor d;
if (_imgName == "_black")
d(GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_BLACK);
else
d = DefaultConstructor(_imgName);

return d.construct();

This code won't work. It could be done with pointers or a temporary variable, but is there an elegant way to do it? Can I declare a class and construct it later on?

You can't trigger the construction later on (after it has been declared), this must happen immediately.

 

Why not do it this way?

if (_imgName == "_black") {
DefaultConstructor d(GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_BLACK);
return d.construct();
}
else {
DefaultConstructor d(_imgName);
return d.construct();
}

 

or this way (which is harder to read, but shorter):

DefaultConstructor d(
_imgName == "_black" ? 
	  GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_BLACK :
	  _imgName;
);
return d.construct();

 

or this way:

std::string imgName(_imgName);
if (imgName == "black") {
  imgName = GlobalRegistry().get("user/paths/bitmapsPath") + IMAGE_BLACK;
}
DefaultConstructor d(imgName);
return d.construct();

[mohij]

I implemented this with a simple check in the Image MapExpression. According to modwiki.net those keywords could also be used as materials themselves, but that doesn't seem to be used. In fact the only keywords I found in the Doom3 materials in use are _currentRender (quite often), _cubicLight (once), _white (twice) and _flat (once).

MapExpression.cpp

Of course the _whatever.bmp files need to be existant, I hope someone has some sort of template for those tiled background text images (like "shader not found" or "no clip").

[greebo]

Cool. I guess what's left to do here is to create these default textures in the bitmaps/ folder and this feature is as good as done.

 

I'll commit your changes soon.

[OrbWeaver]

For things like _white and _black you probably don't even need a BMP, you could just construct the Image from an array with a single RGB triplet (assuming that a 1x1 image works in OpenGL).