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

Nicolai Hähnle (nha) wrote :

> 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.
Thank 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.
Should be fixed now.
> - 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.
Loading the edge.png texture in GameRendererGL does make sense, though
then it would probably be best to make GameRendererGL a "singleton" to
avoid loading the texture multiple times. Or is your new image cache
meant to also cache textures provided by some other part of the code, so
that multiple instances of GameRendererGL would be using the same texture?
> - 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 :)
I'll have to think about it. The problem is that a lot of that stuff is
inherently a two-pass approach.
> - "and should be treated as read-only by derived classes": You made two pointers const, why not the third as well?
The renderer is not supposed to modify egbase or player, but it _is_
supposed to modify the RenderTarget.
> 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.
So sigra's influence is in decline? I actually agree with you in
general, but I believe that most code still passes RenderTargets around
by reference and it's better to be consistent. Besides, it's usually
pretty obvious that you want RenderTargets to be modified.
> - 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?
Leftovers from the original code, will change.
> - 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 :)
Will change.

--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

« Back to merge proposal