Henri Verbeet wrote:
2009/4/22 Tobias Jakobi liquid.acid@gmx.net:
- Why is this in struct ps_compiled_shader? This really looks like
something that should be internal to the backend.
No, that's definitely not the case. The struct's data is used both in GLSL and ARB mode (I'll submit the ARB patches later) - so it makes sense to put it into ps_compiled_shader.
Sure, but it's still internal bookkeeping of the backends. Everything the backend needs from the outside world is the np2_fixup field from struct ps_compile_args.
For ARB mode this is essential since something like the prog_link struct from GLSL doesn't exist there.
It's not terribly hard to introduce something like that. That could mean adding a handle table to the ARB backend, or expanding prgId to be a pointer, but that's ok.
So, is that a suggestion? I don't think that's for me to decide.
You should make clear if I should implement it this way or use another approach. From my point of view it makes sense to share the data, so putting this into ps_compiled_shader is my currently best option.
But if you say, that the current approach is a no-go - I'll try the pointer-approach.
So, how should I do it?
- Writing "GLfloat* const_cache;" is legal C, but really doesn't make
a lot of sense. The * is part of the declarator rather than the base type. An example like "int* p, q;" should make this more obvious.
Your example is bad coding practice IMHO - i would never declare variables with different types on the same line.
The point is that writing "GLfloat* const_cache;" implies the * is part of the type instead of the declarator. C doesn't work that way, it's misleading at best.
IMHO that's splitting hairs.
You could apply the same this for pointer casting, forbidding something like var = (TYPE*)var2; and instead enforce var = (TYPE *)var2;
However, const_cache is now gone - so this shouldn't be a problem anymore ;)
- swz is redundant. "prog->np2Fixup_data->swz & (1 << i)" really isn't
more efficient than "idx & 1"
As far as I can see that doesn't do the same.
Not in the current code, no. But if you store the index of the fixup parameter instead of the index of the packed uniform the parameter is stored in, you can derive the index of the packed uniform as (idx >>
- and the position within that uniform as (idx & 1).
I don't think I can follow you.
I tried some similar approach in the beginning, but it I think it failed if tex1 and tex3 need fixup, but tex2 does not. The approach didn't like "holes" in the indices. However we can't assume that all texture samples that need fixup are consecutive.