Jump to content
The Dark Mod Forums

Recommended Posts

  • Replies 314
  • Created
  • Last Reply

Top Posters In This Topic

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?

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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?

Link to post
Share on other sites

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.

Link to post
Share on other sites

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.

Link to post
Share on other sites

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

Link to post
Share on other sites

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

Link to post
Share on other sites

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?

Link to post
Share on other sites
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();

Link to post
Share on other sites

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

Link to post
Share on other sites

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.

Link to post
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.


×
×
  • Create New...