On Fri, Mar 21, 2003 at 11:49:35PM +0000, Keith Whitwell wrote:
> José Fonseca wrote:
> >Were you thinking going the full way now for the vtxfmt rework? (Sorry
> >for asking and not looking to the code myself, but I don't know exactly
> >what's its state and where to start, and I'm also with my hands full now
> >as you know)
>
> No, it's primarily a rework of the tnl module & it's interfaces. Plus
> removing some cruft.
>
> >In many cases you can simply check the error status. If ctx->ErrorValue
> >hasn't enough (or updated) info about the error, we can extend it
> >elsewhere (e.g., ctx->ReturnValue which would be set on zero on success).
>
> Yes - just pointing out one of the things that I'd come across on the
> same path...
>
> >In some cases most of the code in a API is validation, and the
> >solution you show in api_validate is the most apropriate, but others are
> >which aren't that simple.
>
> Correct.
>
> >NOTE: I'm thinking out loud in the next paragraphs, so you may want to
> >skim quickly through them and read only the last code snippets.
>
> Actually, I'm trying not to do that with your posts anymore...
Well, thanks! :)
> >The only way I see to avoid the duplication of the switch is to break
> >the glEnable in glEnableTexture, glEnableFoo, ... which is not a bad
> >thing. We could have a function table (in the context or passed as and
> >argument to a function _mesa_do_enable which would do the 'switch')
> >whose the index would be 'enum', and there would only by one
> >implementation of glEnable. RadeonEnableTexture would call
> >_mesa_enable_texture, and so on...
>
> I like this idea too. The ideal division for me would be along attribute
> group lines. Similarly you can split up glPushAttrib and glPopAttrib.
> Then (with the init/update-state directions from recent embedded commits)
> attribute state management starts to look "modular"...
I also like the division you made there - one can now make a change in
a attribute group without having to change half places.
> >On other places this is way too much trouble for a small 'switch', and
> >we could simply duplicate the code in Mesa's 'case' statements and
> >eliminate the call to the Mesa API completly.
>
> Wasn't the idea to reduce duplication?
There is the source code duplication, and the run-time execution of
duplicate code paths (which I thought was you main concern). This idea
avoids later, but not the former. In other words, although the Mesa code
is duplicated on the driver, only one of the instances is run on a
single API call - [usually] the driver instance.
> > Note that the state is
> >stored in the context already is exported to the drivers, so everytime
> >we change the state we need to check if the drivers aren't broken, but I
> >confess that I'm still not very confortable doing this, since if we add
> >derived state, things will silently be broken.
>
> You mean if we add some new derived state, then the duplicated code in
> the drivers won't know about it & so won't keep it uptodate?
Exactly.
> >Things would be easier if all these glEnableFoo were inlined and defined
> >in a header, and _mesa_Enable and RadeonEnable would use them as
> >suitable. There would be duplication of the switch statement in the
> >code, but only one of them would be executed at run-time.
>
> Or if each enable enum were in a header.
Not very orthodox, but surely doable and effective.
> Or if we just eat the function call.
With a #define, for example. It seems OK for all circumstances I can
think of.
But as the problem is that the 'switch' code is in the drivers, so
everytime you wanted to add a new 'case' to the 'switch' statement you
had to add it to all drivers, so that they include a new
header/#define/inline whatever...
> I think you're seeing some of the reasons I didn't go all the way with
> this in 3.5...
Indeed.
> > ...
>
> That's not really any better than the current system. I'd rather see
> (for enable) :
>
> _mesa_enable( enum )
> {
> get_context();
> fn = get_enable_fn_from_hash( enum );
> if (fn) fn( enum, GL_TRUE );
> else raise_error();
> }
>
> Then:
>
> bool radeon_enable_texture( enum e, bool b )
> {
> if (_mesa_enable_texture( e, b )) {
> RADEON_FIRE_VERTICES();
> }
> }
>
> bool radeon_enable_polygon_stipple( e, b )
> {
> ...
> }
>
> void init()
> {
> register_enable( GL_TEXTURE_1D, radeon_enable_texture );
> register_enable( GL_TEXTURE_2D, radeon_enable_texture );
> register_enable( GL_POLYGON_STIPPLE, radeon_enable_polygon_stipple
> );
> }
>
> This also works nicely with extensions that may or may not be turned on.
I really liked your idea of the 'get_enable_fn_from_hash' function,
because it allows for the C++ drivers to avoid using a C function table
for the hash, it would replace that same function by another which would
use internally a C++ method functions table, therefore avoiding having
to write wrappers for each function in the hash. (PS: I know that we can
always resort to using normal C functions in C++ but it would definetly
be better if we use methods which can be inherited from other classes,
as many of the things usually done in this sort of routines are done by
all drivers thus can be shared).
> >Or when we need pre-validation:
> >
> > ...
>
> I think this would turn into a pretty big mess over time...
At least it's good we have some options. I don't think it's wise to suddenly
change all APIs to one method or another, as it may show to be a bad
choice after all. glEnable and glGet are clearly good candidates for
experimenting with 'get_enable_fn_from_hash' thing.
José Fonseca
-------------------------------------------------------
This SF.net email is sponsored by:Crypto Challenge is now open!
Get cracking and register here for some mind boggling fun and
the chance of winning an Apple iPod:
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0031en
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
