widelands

Code review comment for lp:~nha/widelands/opengl

opengl
Merge into trunk

Revision history for this message

SirVer (sirver) wrote on 2013-02-08:

Nicolai! The code compiles and runs great on later Mac OS X - I cannot check with old OS X as my wife took the laptop with her to work. The speedup is quite tremendous: on 100x speed terrain rendering takes less than 15% CPU now on my box. Superb work as always from you.

As you might have noticed we took up a review kind of process. The specifics are not clear yet - I just learned on my job that this can be very useful and improve the code significantly. So here are some comments:

- the diff here on the site is borked (it reports merge conflicts). Can you repropose? I think it updates the diff.
- adding the intensity parameter to the load() function seems suboptimal. First it is very specific to OpenGL rendering as SDL has no use for it. Secondly it gives the class a second task (namely loading and then 'flattening' the image). How about moving the edge/road loading and ownership out of graphic into the gameview renderer and giving only the GLTextures the intensity parameter. You could even make a conversion (static) method that could convert an RGB to an intensity texture - this would slow startup by a tad, but would keep this stuff internal to the (gl) terrain rendering - I feel that is a cleaner design.
- Some of the functions (e.g. drawobject(), but also others) are very long and repetitive? Can you find some seams to offload stuff into private or even better static methods/functions? I would not feel comfi to refactor anything in those methods as they stand now :)
- "and should be treated as read-only by derived classes": You made two pointers const, why not the third as well? How about not making those protected members, but instead combine them into a struct RenderState or so and pass this down to the functions doing the work? Your call.
- RenderTarget & dst: I prefer to pass a mutable object by pointer. The reason is that it becomes apparent on the call site if the call might modify the object or not. I realize that you prefer to use reference when an object must be passed (i.e. cannot be NULL), but i feel adding an assert as first line to the function does an equally good job. Your choice of course.
- I love MapviewPixelFunctions. I will wrap some of those for Lua (there are some values hard coded there).
- You pass Point by value regularly - why not use a const reference?
- GameRenderer is a struct, but should be a class. We started to run into trouble with clang complaining about the difference. We now use Class for anything with non trivial methods and struct otherwise. Sorry, you need to type the public: now :)

- the diff here on the site is borked (it reports merge conflicts). Can you repropose? I think it updates the diff.
- adding the intensity parameter to the load() function seems suboptimal. First it is very specific to OpenGL rendering as SDL has no use for it. Secondly it gives the class a second task (namely loading and then 'flattening' the image). How about moving the edge/road loading and ownership out of graphic into the gameview renderer and giving only the GLTextures the intensity parameter. You could even make a conversion (static) method that could convert an RGB to an intensity texture - this would slow startup by a tad, but would keep this stuff internal to the (gl) terrain rendering - I feel that is a cleaner design.
- Some of the functions (e.g. drawobject(), but also others) are very long and repetitive? Can you find some seams to offload stuff into private or even better static methods/functions? I would not feel comfi to refactor anything in those methods as they stand now :)
- "and should be treated as read-only by derived classes": You made two pointers const, why not the third as well? How about not making those protected members, but instead combine them into a struct RenderState or so and pass this down to the functions doing the work? Your call. 
- RenderTarget & dst: I prefer to pass a mutable object by pointer. The reason is that it becomes apparent on the call site if the call might modify the object or not. I realize that you prefer to use reference when an object must be passed (i.e. cannot be NULL), but i feel adding an assert as first line to the function does an equally good job. Your choice of course.
- I love MapviewPixelFunctions. I will wrap some of those for Lua (there are some values hard coded there).
- You pass Point by value regularly - why not use a const reference? 
- GameRenderer is a struct, but should be a class. We started to run into trouble with clang complaining about the difference. We now use Class for anything with non trivial methods and struct otherwise. Sorry, you need to type the public: now :)

« Back to merge proposal