BeginDraw/FinishDraw poorly named

[res]

Migrated from SFbug:764156.

Original description:
Depending upon one's point of view, the BeginDraw() and FinishDraw() methods are either very poorly named or poorly designed. Contrary to what the names imply, in the present design, one calls BeginDraw() any number of times; each invocation sets the desired drawing mode (2D or 3D). At the very end, after all drawing is complete, one calls FinishDraw().

There are at least two methods we could employ to clear up this situation.

1. Choose better names; names which do not imply that the methods nest or that they must be called in pairs. For example, SetupDrawMode() and FinalizeDraw().

2. Re-work the logic so that the methods truly nest, and enforce the rule that they must be called in pairs. In a correctly nesting design, the actual working code of FinishDraw() will be run automatically only when outermost FinishDraw() is invoked. As a convenience, we can introduce a utility class which helps to ensure that these methods are always called as a pair. For example:

class csDrawContext3D
{
private:
iGraphics3D* g3d;
public:
csDrawContext3D(iGraphics3D* p, int flags) : g3d(p)
{ g3d->BeginDraw(flags); }
~csDrawContext3D() { g3d->FinishDraw(); }
};

To utilize this class, simply instantiate it at the point where you would manually invoke BeginDraw(). The corresponding FinishDraw() will be invoked automatically at the end of the enclosing block. For instance:

csDrawContext3D dc(g3d, engine->GetBeginDrawFlags() | CSDRAW_3DGRAPHICS);

Original comments:

Date: 2006-05-07 07:54
Sender: jorritProject AdminAccepting Donations
Logged In: YES 
user_id=1703

One thing to keep in mind is that BeginDraw() is
occasionally used to clear the z-buffer as that is the only
way to do it right now. So perhaps we should add a specific
api call for this case too.

Date: 2006-04-07 08:56
Sender: jorritProject AdminAccepting Donations
Logged In: YES 
user_id=1703

As discussed yesterday on IRC there are however a few
problems with the nested approach. Sometimes it is required
to prematurely flush the contents of the buffer on screen
(for example bugplug needs this to be able to make a
screenshot).

Also the nesting approach is more error prone as one can
forget to call FinishDraw. Using a context wrapper can solve
that problem partially but then there is a potential other
problem with some routine needing to do something in 3D
first and then in 2D. If that routine isn't careful with
scopes it could do something like this:

csGraphicsContext mode3d (3Dmode);
... 3d stuff
csGraphicsContext mode2d (2Dmode);
... 2d stuff

However when the current scope ends I don't suppose it is
guaranteed in which order the 'mode3d' and
'mode2d'
variables will destruct. Or is it?

Additionally one needs to know when the frame ends anyway
since you have to call Print() at the end. So that sounds
like a suitable place to also put the final (and only)
FinishDraw() as well.

Lastly the nested approach is slightly less efficient as one
needs more virtual function calls to the renderer. i.e.
everytime one needs to do something in 2D mode one has to do
a Begin and a Finish. With the simple SetMode() approach one
just has to call SetMode(2D) in the beginning.

Of course on the other hand the nested approach is more
robust in the sense that you can safely call functions and
be 100% sure (if there are no bugs) that the 2D/3D mode will
not be changed.

So I think nested *may* be better but it does have
considerable disadvantages too.

Greetings,

Date: 2006-04-06 23:39
Sender: res2002Project AdminAccepting Donations
Logged In: YES 
user_id=360881

Marten brought up a good point - whether FinishDraw() is
really needed. The idea is that it's functionality could
be
merged into Print().

Date: 2006-04-06 23:29
Sender: sunshineProject Admin
Logged In: YES 
user_id=1767

I suggested nesting in the first place for the reasons which
Frank iterated. The 
most important benefit is that it dramatically improves
modularity by removing 
the necessity of having some supervising entity responsible for
the final 
"FinishDraw"; and thus promotes decoupling, which is A
Good Thing. This leads 
to modules which are more self-contained, and simplifies the job
of module 
authors.

Date: 2006-04-06 15:54
Sender: res2002Project AdminAccepting Donations
Logged In: YES 
user_id=360881

"But why make it nestable in the first place? What do we
gain by that?"

See argument "it's less error-prone" in previous
comment. We
gain that FinishDraw() placement is less problematic.

"Another thing to consider. Why do we even need
BeginDraw()
for switching 2D/3D modes anymore? Cannot the GL renderer
detect this itself and switch to the right mode
automatically depending on the API call that is used?"

GL needs different projection matrices for 2D and 3D
drawing. Also, since currently 2D and 3D drawing reside in
different modules, auto-switching is made more difficult.
Basically, either the canvas somehow notifies the renderer
that 2D drawing is about to happen or the renderer the
canvas that 3D drawing is. Either way means more overhead.

Date: 2006-04-06 15:34
Sender: jorritProject AdminAccepting Donations
Logged In: YES 
user_id=1703

But why make it nestable in the first place? What do we gain
by that?

Another thing to consider. Why do we even need BeginDraw()
for switching 2D/3D modes anymore? Cannot the GL renderer
detect this itself and switch to the right mode
automatically depending on the API call that is used?

Greetings,

Date: 2006-04-06 15:28
Sender: res2002Project AdminAccepting Donations
Logged In: YES 
user_id=360881

What actually speaks for nestability is that it's less
error-prone: Currently, when one module calls FinishDraw()
but some other module draws stuff after that, drawing may
get messed up. OTOH, if you think that some module calls
FinishDraw() but doesn't, it may not get called at all,
and
things are messed up again. By enforcing BD/FB balancing
this problem is eliminated.

My previous suggestion of changing both name and behaviour
actually has some merit: leave the old methods with old
semantics there, but deprecate them; that won't break
clients immediately and they get a chance to update their code.

Date: 2006-04-06 15:23
Sender: res2002Project AdminAccepting Donations
Logged In: YES 
user_id=360881

We could combine the best (ie most annoying) parts of both
approaches: make them nestable *and* change the name.

Date: 2006-04-06 15:18
Sender: jorritProject AdminAccepting Donations
Logged In: YES 
user_id=1703

I would vote against changing behaviour to conform to
nesting. This is a very dangerous change and would break
many apps without warning. I would rather see the names
changed. Something like BeginDraw renamed as SetupDrawMode
and FinishDraw could remain.

Greetings,

Date: 2006-04-06 15:15
Sender: res2002Project AdminAccepting Donations
Logged In: YES 
user_id=360881

If the methods are to truly nest, some more logic to ensure
the correct "state" is needed; e.g.
BeginDraw(3D);
...
BeginDraw(2D);
...
FinishDraw();
- here, the state should be "3D mode" again
FinishDraw();
- state is undefined

So a "mode stack" would have to maintained, and inner
FinishDraw()s would pop a mode and change to it, while only
the outermost one would truly finish.
[Since the current BeginDraw() is a "state change", it
could
be reused, so it would be called by FinishDraw() with the
popped mode.]

Another thing to consider is render-to-texture: What happens
if you call SetRenderTarget() in a nested
BeginDraw()/FinishDraw() block? Where go things you draw if
you set a render target but did not call BeginDraw()? etc.

Date: 2006-04-05 22:51
Sender: neverjade
Logged In: YES 
user_id=13396

I think that this is a good idea.  The current situation is
confusing.  OpenGL has a nested setup, and it makes things
very clear.

Date: 2005-09-19 02:06
Sender: nobody
Logged In: NO 

im not a project maintainer but in my opinion I think any 
advantages to this are blown away by the added 
uneccessary complexity in understanding this. 
 
The current methods are easy enough to understand 
imho. 

[jorrit]

This is an api change. This must be done before 1.0 release. Why not set the milestone then?

[genjix]

I propose: StartDrawing, StopDrawing. Not too radically different but conveys the meaning in a clear concise way.