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

Revision history for this message
SirVer (sirver) wrote :

Not sure if the code in the diff is up to date again... I did a bzr merge and looked at my local copy.

Responses:
> So sigra's influence is in decline?
Erik has not been around for a long time now. He started a job and
mentioned some years back that this will likely make it hard for him to
continue on widelands. I wouldn't say that is 'influence is in decline',
we just develop, learn and change :)

> 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.
Ack.

About get_dither_edge_texture: Yes, approximately. I pushed a branch for merging - I think what you want here is a new TransformedImage or directly using the SurfaceCache. I pushed my branch for review just now, so you can have a look at my design. As you were quicker with the merge proposal though, the fun of fixing the merge conflicts is on me, I guess.

Nitpicking:
- Widelands::Editor_Game_Base const &. Prefer to put const in front, more similar to a pointer. Same for other objects of course.
- class GameRenderer. Make boost::noncopyable?
- in some places inside function you could use const for variables more
  aggressively. This would give the compiler a better chance to
  optimise.
- What about the dynamic_casts? Why not static, we should know the types
  in all cases, right?
- assert(sizeof(basevertex) == 32); We have a compile assert statement
  for this somewhere.

These are just nits from my point of view. So I am completely fine with
merging this as is. Can you do the merge or should I - just remember to
never push your branch to trunk, always merge into trunk and push that.

review: Approve

« Back to merge proposal