mohij 0 Posted June 18, 2007 Author Report Share Posted June 18, 2007 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? Quote Link to post Share on other sites
greebo 86 Posted June 18, 2007 Report Share Posted June 18, 2007 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? Quote Link to post Share on other sites
mohij 0 Posted June 18, 2007 Author Report Share Posted June 18, 2007 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. Quote Link to post Share on other sites
OrbWeaver 662 Posted June 18, 2007 Report Share Posted June 18, 2007 Yes, any replacement of raw pointers with smart pointers is a good step. Quote DarkRadiant homepage ⋄ DarkRadiant user guide ⋄ OrbWeaver's Dark Ambients ⋄ Blender export scripts Link to post Share on other sites
greebo 86 Posted June 18, 2007 Report Share Posted June 18, 2007 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. Quote Link to post Share on other sites
mohij 0 Posted June 18, 2007 Author Report Share Posted June 18, 2007 Ditch means remove completely? Edit: I just checked, and the FileLoader and DefaultLoader are still used. Quote Link to post Share on other sites
greebo 86 Posted June 18, 2007 Report Share Posted June 18, 2007 Yup, if it's unneeded, there's no sense in keeping it. Quote Link to post Share on other sites
mohij 0 Posted June 18, 2007 Author Report Share Posted June 18, 2007 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. Quote Link to post Share on other sites
greebo 86 Posted June 18, 2007 Report Share Posted June 18, 2007 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. Quote Link to post Share on other sites
mohij 0 Posted June 20, 2007 Author Report Share Posted June 20, 2007 What is better,Introducing a shared_ptr<Image> and another shared_ptr<RGBAImage> and using the ones I need, oronly introducing shared_ptr<Image> and cast to RGBAImage when needed? Quote Link to post Share on other sites
mohij 0 Posted June 20, 2007 Author Report Share Posted June 20, 2007 btw. Should I open a new topic for this? Quote Link to post Share on other sites
greebo 86 Posted June 20, 2007 Report Share Posted June 20, 2007 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. Quote Link to post Share on other sites
mohij 0 Posted June 20, 2007 Author Report Share Posted June 20, 2007 I guess that's an exhausted answer, thanks. Quote Link to post Share on other sites
mohij 0 Posted June 20, 2007 Author Report Share Posted June 20, 2007 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? Quote Link to post Share on other sites
OrbWeaver 662 Posted June 20, 2007 Report Share Posted June 20, 2007 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. Quote DarkRadiant homepage ⋄ DarkRadiant user guide ⋄ OrbWeaver's Dark Ambients ⋄ Blender export scripts Link to post Share on other sites
greebo 86 Posted June 20, 2007 Report Share Posted June 20, 2007 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. Quote Link to post Share on other sites
mohij 0 Posted June 21, 2007 Author Report Share Posted June 21, 2007 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 Quote Link to post Share on other sites
greebo 86 Posted June 21, 2007 Report Share Posted June 21, 2007 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. Quote Link to post Share on other sites
mohij 0 Posted June 21, 2007 Author Report Share Posted June 21, 2007 Heya, syncing fixed the bug. :-) Quote Link to post Share on other sites
greebo 86 Posted June 21, 2007 Report Share Posted June 21, 2007 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 Quote Link to post Share on other sites
mohij 0 Posted June 23, 2007 Author Report Share Posted June 23, 2007 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? Quote Link to post Share on other sites
greebo 86 Posted June 23, 2007 Report Share Posted June 23, 2007 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(); Quote Link to post Share on other sites
mohij 0 Posted June 23, 2007 Author Report Share Posted June 23, 2007 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.cppOf 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"). Quote Link to post Share on other sites
greebo 86 Posted June 24, 2007 Report Share Posted June 24, 2007 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. Quote Link to post Share on other sites
OrbWeaver 662 Posted June 24, 2007 Report Share Posted June 24, 2007 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). Quote DarkRadiant homepage ⋄ DarkRadiant user guide ⋄ OrbWeaver's Dark Ambients ⋄ Blender export scripts Link to post Share on other sites
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.