Re: Is this bad coding ?
mast4as wrote:
Okay let me first explain what I am trying to solve. (...)
Here is the problem. Lets imagine all the threads render the same
object. Therefore they will access 'shader' potentially at the same
time. Which means that if the threads need to change anything in
'shader' before it calls 'shade()' the application won't be thread
safe;
class Shader
{
float param;
color shade(...) { color c = 1; return param * c; }
}
objIntersect->param = threadId; ///< NOT THREAD SAFE
pixelColor = objIntersect->shade(); ///< NOT THREAD SAFE because by
the time shade() is executed
///< param may have
been changed by another thread !!!!
Your example is quite strange because you call the Shader member
"param". If it is indeed a parameter, then by all means pass it as a
parameter (or do we call it argument?) to the shade function!
So basically the only workaround that I can see is to dynamically
allocate an instance of the class Shader each time there's an
intersection but that forces me to do a new/delete potentially a few
hundred millions times (yes that many times depending of the geometry/
shading complexity).
if (intersection) {
Shader *shd = new Shader();
shd->Kd = threadId;
pixelColor = shd->shade();
delete shd;
}
This is not C++ code, but more like Java code. You do *not* allocate
everything on the heap in C++.
if (intersection) {
Shader shd;
shd.Kd = threadId;
pixelColor = shd.shade();
}
.... no dynamic allocation necessary here. (If you even need an object or
would be better off with a free function is hard to tell from the
example code).
YES I KNOW I can pass threadId to the shade function as parameter but
lets say I CAN'T (because the code is in fact more complicated and
that I simple can not doit it;-).
So I am trying to find a solution to this problem. I tried to
prototype something very quickly and I came with this (see code
below). But I am afraid IT'S UGLY from a coding point of view. Because
I am passing the parameters of the shader (packed in a structure) as a
void pointer to a function and passing them back to the shader as a
void pointer and casting them to their original type. That sounds bad
hey ! But I timed that application and it's much much faster. Is it
really bad or acceptable ? Does anyone see a better way of doing
this ?
typedef struct color { float r, g, b; };
(...)
Skip the typedefs. They are a C thing!
class Shader
{
public:
Shader() {}
virtual ~Shader() {}
virtual color surface(const ShadingInfo &) const = 0;
virtual void evaluate(const void *) const = 0;
};
color illuminate(const RenderContext *, const Shader *shd, const void
*);
class Plastic : public Shader
{
class PlasticShaderParams : public ShaderParameters
{
public:
float Kd, Ks;
};
const RenderContext *rc;
public:
Plastic(const RenderContext *rctx) : rc(rctx) {}
color surface(const ShadingInfo &shadingInfo) const
{
color c = { 0, 0, 0 };
PlasticShaderParams shdParams;
shdParams.Ks = .4f;
shdParams.Kd = .6f;
illuminate(rc, this, &shdParams);
return c;
}
void evaluate(const void *params) const
{
PlasticShaderParams *shadParams = (PlasticShaderParams*)params;
//fprintf(stderr, "Kd : %f\n", shadParams->Kd);
}
};
color illuminate(const RenderContext *rc, const Shader *shd, const
void *shdParams)
{
color c = { 0, 0, 0 };
shd->evaluate(shdParams);
return c;
So. Apparently evaluate() and illuminate() take shader parameters as
arguments. Why on Earth are you then passing a void* ?
Pass a ShaderParameters* and - if you really need polymorphism here -
then cast that down to PlasticShaderParams* by using dynamic_cast.
Hope my comments have been of some help.
br,
Martin
--
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]