Nice work! The InMemoryImage is still a bit of a hack, but all the rest is now
much cleaner than it used to be. I like the overall architecture a lot. Some
comments on the details:
1. You use non-Doxygen formatted comments a lot in places where it would be
nicer to have Doxygen comments. (The Doxygen build is overall not in the best
shape, but still...)
2. constants.h: In C++, one is supposed to include cstdint etc. instead of
stdint.h etc. (unless perhaps there are some C++ compilers where those files do
not exist?)
3. editor/editorinteractive.cc: around line 155, the c_str() in immname.c_str()
is not necessary
4. AnimationGfx: Why is the image cache stored again in instance of this class
instead of just accessing it via g_gr like everywhere else?
5. graphic/graphic.h: The comment above the declaration of Graphic is outdated,
maybe it's time to change that ;)
6. Compiling with g++, there are many warnings of the type "declaration of XXX
shadows a member of 'this'". I guess you're using clang on Mac OSX which
doesn't report such warnings? Here are some examples:
[ 1%] Building CXX object
src/CMakeFiles/widelands_all.dir/graphic/image_transformations.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::TransformedImage::TransformedImage(const string&,
const Image&, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:287:91:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::ResizedImage::ResizedImage(const string&, const
Image&, SurfaceCache*, uint16_t, uint16_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:319:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::GrayedOutImage::GrayedOutImage(const string&, const
Image&, SurfaceCache*)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:341:89:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::ChangeLuminosityImage::ChangeLuminosityImage(const
string&, const Image&, SurfaceCache*, float, bool)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:358:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc: In
constructor ‘{anonymous}::PlayerColoredImage::PlayerColoredImage(const string&,
const Image&, SurfaceCache*, const RGBColor&, const Image&)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/image_transformations.cc:381:3:
warning: declaration of ‘hash’ shadows a member of 'this' [-Wshadow]
[ 3%] Building CXX object
src/CMakeFiles/widelands_all.dir/graphic/render/gl_surface.cc.o
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc: In
member function ‘virtual void GLSurface::draw_line(int32_t, int32_t, int32_t,
int32_t, const RGBColor&, uint8_t)’:
/home/nha/dev/widelands/repo/autocache/src/graphic/render/gl_surface.cc:157:87:
warning: declaration of ‘width’ shadows a member of 'this' [-Wshadow]
--
https://code.launchpad.net/~sirver/widelands/autocache/+merge/147559
Your team Widelands Developers is requested to review the proposed merge of
lp:~sirver/widelands/autocache into lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
