On Monday 17 May 2010 22:29:24 Timo wrote:
> Hi,
>
> after a long restructuring process my opengl branch is getting stable
> now. There is still lot to do and it needs some clean up. But you can
> see how I though of the implementation. If someone have good ideas in
> someplace or see some conceptional faults it's now the right time to
> mention them.
I've looked at your branch now, admittedly briefly, but anyway here goes. Some
of these you're probably aware of yourself, but I've listed them here anyway.
1. I like the overall structure of what you've done with the code very much.
2. I haven't noticed any regressions in non-OpenGL mode so far.
3. Your coordinate computations in draw_rect were wrong; OpenGL pixel centers
are at half-integers, so when drawing lines that's where you have to put the
coordinates.
4. Your color value conversions were wrong. To go from a [0,255] valued color
value to a floating point value, you have to divide by 255, not by 256. Even
better, just use the ColorNub variants of OpenGL entry points.
(I've fixed the cases of 3+4 that immediately caught my eye in my branch
lp:~nha/widelands/opengl; this also fixes a rendering bug with how the soldier
info icons are rendered)
5. Without the above, the only rendering errors I noticed were the lack of
scaling of the main menu background, and the lack of terrain blending. Then
again, I haven't done too much testing yet.
6. RenderTarget::draw_line: add a virtual draw_line to Surface to avoid the
ugly USE_OPENGL ifdef and reliance on if (g_opengl)
7. I'm a bit worried that you're allocating too many textures. If I understand
the code correctly, every frame of every animation will end up in its own
texture. Common wisdom is that OpenGL works better the fewer you change
textures, so this is "somewhat" unproductive.
A typical trick is to suballocate a larger texture into smaller chunks. I even
have some code lying around somewhere that does that, and other projects
should have code for that as well (e.g. Quake does it for lightmaps, though
that may have been inside the map compiler, I don't remember the details).
8. The style of OpenGL usage in the code is rather deprecated. Modern OpenGL
favours using vertex arrays (and VBOs) over immediate mode. This is
particularly relevant because OpenGL ES doesn't even *have* immediate mode.
Not sure if Android or Maemo based devices have full OpenGL or only OpenGL ES,
but to make portability easier, it would be preferable to use less immediate
mode.
Of course, at the same time we should work well with the open source OpenGL
drivers out there, so going the full OpenGL 3.x route isn't really an option
yet, either, but at least VBOs are widely supported.
9. On a related note, I would put the separation of terrain rendering at a
higher level than what you did. You put the separation at the individual field
level, which works quite well (after all, we really have relatively few
triangles in a scene, and a modern graphics card is quite bored), but it goes
contrary to the recommended OpenGL use, and we could use CPU more efficiently
by doing OpenGL terrain rendering entirely differently.
Instead of having a set of OpenGL calls for each field (and thus each
triangle), we could fill vertex buffers with the information required to
render the whole scene, and then render the entire scene (at least the terrain
itself) at once with a single OpenGL call (or potentially one OpenGL per
terrain type).
So, lots of comments. My final verdict is that, regardless of all those
comments, the branch should be merged ASAP, except for regression in non-
OpenGL mode. I'm quite happy with how the code looks, actually.
Any of the comments I had can then still be addressed when the code is in
trunk, I don't see a problem with that.
To give you another data point, I've tested with a Radeon HD 4800, fglrx
driver. Once I get to work more with that code I can test it on a whole array
of different Radeon cards ;)
> It would be good to get some feedback now. On which systems does it work
> on which not? I spent some time to get it working on my laptop (Raedeon
> mobility 9000, 32MB, x11 open source driver, opengl 1.3) so it should
Okay, so low OpenGL versions seem to be unfortunately widespread still :/
Anyway, vertex arrays are also supported by OpenGL 1.3, just no VBOs, but it's
relatively easy to set things up so that the same code path works with and
without VBOs.
cu,
Nicolai
--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
|