   Simple Image Library
  The alpha version of the Simple Image Library (glaux/IPicture replacement) is now online here(1.77Mb). Have a play, but don't distribute it 'til it's released properly. Tell me if you have any problems, or dislikes about it [grin]. It uses a simplified version of the SDK coding standards, and is written for C++ beginners.Gregs   
  Looks very nice! But why does nobody make projects for us Dev-Cpp users? [bawling]Well, when you have released it we might use it within the SDK, cause it's really simple to use...;Quote:Original post by CasteBut why does nobody make projects for us Dev-Cpp users? [bawling]Probably because the Visual Studio IDE is so easy to work with. [wink];  Before I start ripping your code apart I just want to make sure you understand that I do appreciate you're taking the time to write this code and make it available.  I'm not trying to put you down with any of my comments, just trying to ensure that the revamped NeHe is of the highest quality we as a group of developers are capable of.  I'm not sure exactly what audience you're aiming this at, so I shall make my comments assuming that it's aimed at absolute beginners and not experienced developers, not that this will make a huge difference (should have read your initial post more carefully [lol]).  I shall comment on a file-by-file basis in multiple posts because I don't have time to go through the whole lot in one sitting.sil.cpp:I'm not convinced that making sil a predominantly C api is the best course of action.  Yes, having a C api is advantageous because of compatability, but it's usually fairly trivial to wrap a C api around a C++ api anyway - you've pretty much done that already with sil wrapping the ImageManager, you've just not given the option to use sil as a C++ api in its own right.  Providing a C++ api has the advantage of eliminating several classes of user error that are pretty much unavoidable in C.  Having the api return a std::string of the last error for example prevents users storing the error string and then calling another sil function and potentially invalidating the stored value.static at global or namespace scope is currently deprecated in C++ in favour of unnamed namespaces, although there has been a suggestion to reverse this deprecation.Potential "order of non-local static objects" initialisation issues if sil is not built as a dll and a sil function is called from static code before the global image manager has been initialised.silConvertImage is a potentially dangerous function.  It's far too easy to misuse by passing a wrong-sized array.  If the overall design does not change then this function would be much better returning a handle to a converted image instead.The whole Create/Delete paradigm would be much better implemented using RAII, assuming a C++ api.  Such a change would eliminate an entire class of user errors.Σnigma   ;  Enigma:Thanks for the comments, and reminding me to put a Dev-C++ project in there(Caste).Please allow me to explain why I made the design decisions I did:C API:This is because it was designed as a predominantly Win32 DLL originally. I would have preferred to have just used the classes, but if the library is presented just as an interface, different compilers would have problems with it (Especially Borland compilers). If it has a plain C api, it makes it trivial for the built library to be imported into Borland C++, Borland Delphi, python, etc.Static Globals In DLLs:As with the C API, this comes from my experience in writing device drivers for medical equipment. At first glance, it would make more sense to allocate them in DLLMain, but as well as causing portability problems, it simply doesn't work. Believe it or nor DllMain is *not* always called.SILConvertImage:Is an evil function, I agree, but in an C API, it is probably the only way to make sure that the library user handles the allocation. It is really only there for people who want to use non-standard colour formats, or DirectX [grin]Create/Delete:I agree completely, however (in my experience) people who use OpenGL tend to have a C mindset, and like to have their helper libraries work with a similar interface to OpenGL. If you look at the code, though, delete is mainly just there for people who want more control. The library will always get rid of undeleted images anyway.Also if you read the comments, it is meant for *absolute* beginners [grin][Edited by - gregs on June 7, 2006 7:01:38 AM];  ImageManager.cpp:#if !defined(_IMAGEMANAGER_H).  Names beginning with an underscore followed by a capital letter are reserved by the C++ standard for compiler and standard library implementors only.  IMAGEMANAGER_H or SIL_IMAGEMANAGER_H should be perfectly adequate replacements.Why do you use an unsigned (int) to store the string length, rather than just a std::string::size_type?  The latter will always be valid, the former not always (although virtually always).There's no point in incurring the overhead of an exception if you're going to catch it after it's unwound out of the new call.  You might as well just call the nothrow form of new and test for nullness.  It's less typing and more probably more efficient.std::string lowerfilename = filename;std::transform(filename.begin(),filename.end(),lowerfilename.begin(),tolower);std::string suffix = lowerfilename.assign(lowerfilename,strlength-3,3);How about:std::string suffix = filename.substr(strlength - 3);std::transform(suffix.begin(), suffix.end(), suffix.begin(), tolower);Shorter, clearer, more efficient.The magic number '3' should be given a symbolic name.  New programmers learn by imitation - let's set a good example.Why suffix.compare instead of just suffix ==?Make the suffix.compare if statements into an if-else if chain - better expresses intent and is more efficientYou have a lot of repetition of error checking when you call FromID, just so you can set the function name in the error string.  How about passing the function name to FromID and letting it set the error string?  I'm not 100% convinced about this but it's the best solution I can find which fits your design.Why do you use char as your image data type?  char is the textual data type and may be signed or unsigned, depending on compiler.  Better to explicitly make the image data type signed char or unsigned char.  I'd vote for the latter.  Unless C89 lacks signed/unsigned char types.Your ClearAll implementation is unusual, why not just a loop through all items to delete the image pointers and then a clear on the map?Σnigma   ;Quote:Original post by EnigmaImageManager.cpp:#if !defined(_IMAGEMANAGER_H).  Names beginning with an underscore followed by a capital letter are reserved by the C++ standard for compiler and standard library implementors only.  IMAGEMANAGER_H or SIL_IMAGEMANAGER_H should be perfectly adequate replacements.I agree. I was being lazy , and just using the StarUML generated files.Quote:Original post by EnigmaWhy do you use an unsigned (int) to store the string length, rather than just a std::string::size_type?  The latter will always be valid, the former not always (although virtually always).That was for consistency + I wasn't sure if it was a good idea to introduce beginners to internal types at that point. Later on I used internal types myself, so I agree that should be changed.Quote:Original post by EnigmaThere's no point in incurring the overhead of an exception if you're going to catch it after it's unwound out of the new call.  You might as well just call the nothrow form of new and test for nullness.  It's less typing and more probably more efficient.That's what I did originally. Then I tried compiling under vs6, which doesn't appear to believe that a nothrow extension exists.Quote:Original post by Enigmastd::string lowerfilename = filename;std::transform(filename.begin(),filename.end(),lowerfilename.begin(),tolower);std::string suffix = lowerfilename.assign(lowerfilename,strlength-3,3);How about:std::string suffix = filename.substr(strlength - 3);std::transform(suffix.begin(), suffix.end(), suffix.begin(), tolower);Shorter, clearer, more efficient.I agree, much tidier. That particular bit of code was the result of me trying to satisfy several STLs with differing levels of standardisation. In the end it turned out a bit of a mess. The lesson I took away from this was "don't try to be clever if you haven't set vs6 to use STLPort", which is usually my only way of using it.Quote:Original post by EnigmaThe magic number '3' should be given a symbolic name.  New programmers learn by imitation - let's set a good example.Why suffix.compare instead of just suffix ==?Make the suffix.compare if statements into an if-else if chain - better expresses intent and is more efficientI agree. See above.Quote:Original post by EnigmaYou have a lot of repetition of error checking when you call FromID, just so you can set the function name in the error string.  How about passing the function name to FromID and letting it set the error string?  I'm not 100% convinced about this but it's the best solution I can find which fits your design.I agree again. That was a left over from an earlier design.Quote:Original post by EnigmaWhy do you use char as your image data type?  char is the textual data type and may be signed or unsigned, depending on compiler.  Better to explicitly make the image data type signed char or unsigned char.  I'd vote for the latter.  Unless C89 lacks signed/unsigned char types.That was to avoid the void * type from the OpenGL texture interfacing functions. unsigned char * should be fine as that's what the Image class uses.Quote:Original post by EnigmaYour ClearAll implementation is unusual, why not just a loop through all items to delete the image pointers and then a clear on the map?ΣnigmaI agree. I just thought I'd tidy up as I went along [grin]btw I'm rewriting the image class (now classes) after our discussion on IRC.G;  The latest version is now on the website.   ;  Hey gregs, did you succeed in creating the library using Dev-Cpp? 'Cause I get ton's of linker errors because it somehow has problems with zlib.I rebuilt zlib using the source you added but that didn't change anything.Working with Linux didn't cause problems..And by the way, change the Include-path from "C:\Documents and Settings\Gregs\..hmhm..\sil\include" to "../../include", otherwise some users will wonder why it doesn't work![Edited by - Caste on June 17, 2006 9:53:23 AM];  I thought I'd fixed that :(btw, you need the DevPacks of ZLib, libPng & libjpeg installed.   