Stefan Dösinger wrote:
This doesn't look bad on the surface, but I have two concerns:
1. Does not scale to more complex support checks. I suspect you'll have to have to rewrite this as soon as a case comes up that requires more than a single support check to establish support. A function pointer may help here. It would also be interesting to look at similarities and differences with the ffp pipeline replacement - which is essentially the same thing on a much larger scale, where multiple states are replaced rather than a single state. Can these support checks handle the case where an extension would control replacing a group of states ?
2. Not necessarily relating to this patch, but in general I would be wary of trying to replace every if check with a pointer. Splitting on extension support is probably ok.
Ivan
- Does not scale to more complex support checks. I suspect you'll have
to have to rewrite this as soon as a case comes up that requires more than a single support check to establish support. A function pointer may help here. It would also be interesting to look at similarities and differences with the ffp pipeline replacement - which is essentially the same thing on a much larger scale, where multiple states are replaced rather than a single state. Can these support checks handle the case where an extension would control replacing a group of states ?
A function pointer is a nice idea actually. I don't think we'll ever need more than one extension per state line though, and the current scheme can happily deal with two optional extensions(as a later patch dealing with conditional NP2 textures will do). I don't expect that any new function will come around that needs two extension checks at the same time since the fixed function pipeline in both gl and d3d is in its final state and no new features will come up, and shaders are well-standardized.
IMO the current way is easier to read, and until we need a more flexible solution I'd pledge to keep the current code, since no matter what we(I) would have to rewrite it. It is a pain to rewrite/modify a set of patches so I don't want to do it unless we're certain we'll need it
- Not necessarily relating to this patch, but in general I would be
wary of trying to replace every if check with a pointer. Splitting on extension support is probably ok.
It is a performance consideration. The point of doing this is to coalesce if checks into a function pointer abstraction layer that exists anyway. Those if checks can be evaluated at device creation time, so there's no need to run through them every time the state is applied.
The bigger picture is that we have 3 function pointer abstraction layers we can't get rid of: The vtables(we really want inheritance), the state table and the shader backend. My idea is to merge as many if checks as possible that can be determined at object creation time into these anyway-existant abstraction layers. (Of course with the vtables that's a bit ugly, unless we generate vtables dynamically)
Stefan Dösinger wrote:
A function pointer is a nice idea actually. I don't think we'll ever need more than one extension per state line though,
You already have this case:
} else if(GL_SUPPORT(NV_REGISTER_COMBINERS) && GL_SUPPORT(NV_TEXTURE_SHADER2)) { return &nvts_fragment_pipeline;
IMO the current way is easier to read,
I don't know about that - a pointer called "is_<something>_supported" seems just as readable to me.
and until we need a more flexible solution I'd pledge to keep the current code, since no matter what we(I) would have to rewrite it. It is a pain to rewrite/modify a set of patches so I don't want to do it unless we're certain we'll need it
I don't feel strongly about it either way, but the argument that "it will never happen" doesn't seem very convincing. "It will compile several ms slower" is probably a better case to make, but at Init3D-time we probably don't care how fast it is.
Ivan
2008/7/16 Ivan Gyurdiev ivg231@gmail.com:
I don't know about that - a pointer called "is_<something>_supported" seems just as readable to me.
There's certainly the option to use that instead in the future, but for the moment I think the single extension check should work well enough for the majority of cases.
You already have this case:
} else if(GL_SUPPORT(NV_REGISTER_COMBINERS) &&GL_SUPPORT(NV_TEXTURE_SHADER2)) { return &nvts_fragment_pipeline;
Well, that is selecting the fragment pipeline implementation, which does more than just picking a single state. It's mainly for the fragment_enable() callback. Of course I could handle all the code choosing using one table with many different lines with extension information. I don't think that is useful though.
The difference between the pipeline interface selection and the state table extension information is that the pipeline implementations have entirely different codepaths(There's little in common between the nvts and atifs codepath, for example), while the per-line extension selection cares about minor differences in an otherwise consistent object(e.g. do we need NP2 fixups or do we not. This doesn't turn the vertex pipeline implementation upside down like choosing between glsl or fixed function does)
-
H. Verbeet -
Ivan Gyurdiev -
Stefan Dösinger