[Mesa-dev] how to fix progs/demo/geartrain.c?

[Keith Whitwell]

On Sat, 2010-08-07 at 08:01 -0700, Christian Authmann wrote:
> Hi,
> 
> it all started with a harmless looking bug report for r300g on phoronix:
> 
> > http://img227.imageshack.us/img227/8582/bugx.png
> > http://www.phoronix.com/forums/showthread.php?t=24950&page=13
> 
> as I noticed, that bug was present on nvidia too:
> > http://www.authmann.de/tmp/geartrain_nvidia.png
> 
> so I dug into geartrain.c to figure out whether it's the drivers' or the
> testcase's fault.
> 
> Long story short, this is how it *should* look after fixing geartrain:
> http://www.authmann.de/tmp/geartrain_nvidia2.png
> http://www.authmann.de/tmp/geartrain_nvidia3.png
> 
> Issues I've identified:
> 
> 1) loading the .dat file is a mess, uses uninitialized variables, no
> error checking etc. Probably makes this testcase useless for
> valgrind-tests against mesa / drivers.
> 
> 2) the normal vectors are calculated in a way that's valid for the next
> QUAD submitted inside each GL_QUAD_STRIP, which would be perfect for
> glShadeModel(GL_FLAT). But since glShadeModel() is never called, the
> default GL_SMOOTH is used. As a result, shading is slightly off and the
> gears' teeth aren't sharp. It's actually visible on all the gears if you
> look close enough.
> 
> 3) the two beveled gears in the back are terribly broken:
> 3a) the author didn't quite decide which values to use where, leading to
> inconsistent radii on different parts of the gear. This results in
> broken geometry. (Easy to notice if you move the camera).
> 
> 3b) on the outer teeth-polygons of the beveled gears, the faces are
> flipped: the BACK faces are on the outside of the gear. The normals
> still point outwards of the gear, which cannot give good results.
> Especially not without calling glLightModel(GL_LIGHT_MODEL_TWO_SIDE, 1).
> 
> 3c) it's using GL_QUAD_STRIPs, but some of the quads aren't planar on
> the beveled gears. IIRC there's no "correct" way to render and shade
> nonplanar QUADs (but I'm not fluent with the opengl standard), making
> this testcase possibly less useful.
> 
> 
> Now IMHO this should be changed to look "right", to prevent false
> bug-reports or driver-developers implementing workarounds to render it
> correctly.
> 
> But on the other hand, it's a test-case, and it must continue to test
> whatever it's supposed to test.
> 
> I don't mind doing the fixing, but I'd like some feedback as to where
> this program should go.
> 
> Issue 1) can be fixed quickly by getting rid of the external .dat file,
> using static initializers for the structs (struct gear g = {1, 2, 3}).
> It could be fixed with more effort by rewriting the loading routines. Or
> it could be ignored if valgrind tests aren't done anyway.
> 
> Issue 2) is adding a simple call to glShadeModel(GL_FLAT), unless of
> course it's supposed to test smooth shading. In that case, calculation
> of the normal vectors can be adjusted to look better. Which solution is
> preferred?
> 
> Issue 3) can be trivially fixed by removing the broken beveled gears ;)
> Another fix would be to render it with sane methods, as I did for the
> screenshot above. The third option would be to continue doing weird
> stuff, but changing it in a way that makes the result look correct.
> The third one might be the most useful for a test-case.
> 
> Issue 3a) is just "wrong" vertex positions, but doesn't use weird GL
> options at all. A proper fix would be in order (and is done).
> Issue 3b) can continue to test proper shading of backfaces, it'd just
> have to call glLightModel(GL_LIGHT_MODEL_TWO_SIDE, 1) and turn the
> normals around. Right?
> Issue 3c) will again depend on the standard. It looks OK anyway, unless
> a quad is watched at a straight angle. Just keep them non-planar to see
> if the driver can cope?
> 
> 
> Please let me know where you'd like your testcase and I'll make sure it
> gets there. :)
> 
> 
> One last thing, I didn't find geartrain.c on
> cgit.freedesktop.org/mesa/mesa/, so I picked it from the 7.7 release
> package. Where did the tests go?

It's not a test case in the sense that it is exercising a specific
rendering path and must never budge from that.  I'd suggest making
changes that keep the basic appearance (number of gears, straps, etc) of
the demo as it is now, but fixing the glitches.

It sounds like you've done a very detailed analysis of this demo - for
my part I just figured out some years ago that it wasn't quite right &
learned to ignore the glitches...

Keith