#48 kill immediate mode drawing

closed-accepted
Klauss++
None
5
2013-01-21
2012-12-14
logzero
No

This was more of a refactoring/cleanup experiment. The idea was to get rid of immediate mode drawing (glBegin/glEnd). I chose the most simple option and replaced it with glDrawArray, wrapped in a GFXDraw call. The interface is low level, but gets the job done with a minimum of changes I think. I didn't do any explicit profiling. It should be at least as fast as the previous code, does less glCalls. The whole gfx code could use some more state caching though.

Posting here in the hope it might be useful in some way.

Discussion

  • logzero

    logzero - 2012-12-14

    First patch rev missed one file, upped fixed one (v2).

     
  • Klauss++

    Klauss++ - 2012-12-16
    • assigned_to: nobody --> klaussfreire
     
  • Klauss++

    Klauss++ - 2012-12-16

    Huge patch, hard to review. It will take some time.

    From what I've seen, it looks good overall. I'm wondering though which tests have you performed. Reason I'm asking, is, I know begin/end API calls are internally buffered by most decent drivers, whereas array calls aren't. Unwittingly, this patch may be hitting the array APIs in a much sub-optimal way by using it as a replacement of begin/end calls.

    What I've seen doesn't ring any bells yet, but I'll have to review carefully.

     
  • logzero

    logzero - 2012-12-16

    Yeah, it was an all or nothing change. Luckily 95% of it is boilerplate conversion.

    As to glDrawArrays performance. The most naive version would be(from opengl.org):
    void glDrawArrays(glenum mode, glint first, glint count)
    {
    glBegin(mode)
    for i in 0:count-1
    {
    glVertexfv(gl_vertex_array_pointer+(first+1)*vertex_size)
    }
    glEnd()
    }

    I don't think you can get slower than that. The motivation was not perf though, but getting rid of deprecated functionality and reducing that state machine stuff.

    One could try some more clever things of course (use more vbo like interface, add optional vbo support). But that would need more work (require some batching maybe).

     
  • logzero

    logzero - 2012-12-18

    patch split up into digestible chunks

     
  • logzero

    logzero - 2012-12-18

    I've upped gfxdraw.zip which contains 23 separate patches. The patches depend on gfxdraw.patch only, can be applied out of order.

    gfxdraw_v2.patch is the final version of gfxdraw.patch which removes support GFXBegin/End an co, is to be applied instead of gfxdraw.patch.

     
  • Klauss++

    Klauss++ - 2012-12-18

    Cool.

    For the record, I agree with you about the benefit of removing deprecated API usage. It might even help us get rid of those nifty crashes on crappy drivers. So in general, I think this is a good patch.

    I'm just wary of applying it blindly, because it could hit performance rather negatively and rather steeply if done in the wrong place in the wrong way.

    Perhaps I could apply patch-by-patch, as I review them, now that they're separate patches.

     
  • Klauss++

    Klauss++ - 2013-01-01

    I think the bubble_display one is one of the patches I think will bring some trouble with nVidia drivers. From my experience, nVidia doesn't like repeated calls to array APIs, because there's no much buffering involved in those API calls, and it results in way too many "roundtrips" to the GPU.

    This one I think requires some more refactoring. DrawTrack should accumulate the vertices, and perhaps a BeginDrawState(), DrawTrack(), EndDrawState() should be implemented, to be able to buffer everything into a single array call.

     
  • Klauss++

    Klauss++ - 2013-01-13

    I have a question, regarding the cockpit subpatch... any reason why you changed the linestrips into lines? I don't see any...

     
  • logzero

    logzero - 2013-01-14

    About the bubble_display stuff. Are you seeing any issues or have a link, just curious. This is client side drawing stuff, driver will copy the passed data. I would be surprised if it had any issues, wouldn't use a buffer internally to push that stuff to the GPU. It works fine on intel/ati hw.

    About linestrips vs lines. I have been considering to get rid of linestrips altogether to reduce primitive types, have a simpler interface. This could be one of the remainders I guess, haven't looked at the patch for some time now.

     
  • Klauss++

    Klauss++ - 2013-01-14

    I didn't try the bubble patch, I was reviewing it and saw it as a pottential problem with a not-too-difficult solution (I'll do the buffering myself, the VertexBuilder I committed recently would make it trivial). I don't have links either, it's been common knowledge (methinks) from yore that the glBegin/glEnd API did a lot of internal buffering to be acceptably fast, whereas the array APIs provide more direct access to the hardware. I'm pretty sure it's in an nVidia presentation somewhere, but I'd have to google a lot to find it.

    About linestrips, that's fine, but not the way it's done. If you wanted to do linestrips with lines, you'd have to use DrawElements instead of replicating vertices, as replicating vertices hurts the vertex cache. It might amount to a lot on intel drivers, that do vertex processing on the CPU. I'll try to leave the strips there and commit, and worry about a DrawElements interface later.

     
  • Klauss++

    Klauss++ - 2013-01-21

    Everything committed.

     
  • Klauss++

    Klauss++ - 2013-01-21
    • status: open --> closed-accepted
     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks