http://codereview.appspot.com/766041/diff/1/7
File src/liboslexec/bsdf_diffuse.cpp (right):

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
Wouldn't it be better to just call the parent's method rather than
defining base_mergeable?

http://codereview.appspot.com/766041/show

On Mar 25, 2010, at 4:38 PM, <aco...@...> <aco...@...> wrote:

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
Wouldn't it be better to just call the parent's method rather than
defining base_mergeable?

But then if somebody forgets to provide a closure subclass with a mergeable method, it will revert to the parent class, which will be wrong, wrong, wrong, it will say it's mergeable without being aware of any subclass-specific parameters. I wanted the base class's method to either be "return false" (never merge, always correct even if the subclass fails to provide the method) or to be pure virtual (force the subclasses to provide the method).

And yet, I still needed a way for the subclasses to say "and also check the stuff from the parent class".

So that's how I ended up with a pure virtual mergeable() in the base class, and a separate method called base_mergeable().

If you have a more elegant idea, I'm all ears.

--
Larry Gritz
l...@...

On Fri, Mar 26, 2010 at 1:08 PM, Larry Gritz <l...@...> wrote:

On Mar 25, 2010, at 4:38 PM, <aco...@...> <aco...@...> wrote:

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
Wouldn't it be better to just call the parent's method rather than
defining base_mergeable?

But then if somebody forgets to provide a closure subclass with a mergeable
method, it will revert to the parent class, which will be wrong, wrong,
wrong, it will say it's mergeable without being aware of any
subclass-specific parameters.  I wanted the base class's method to either
be "return false" (never merge, always correct even if the subclass fails to
provide the method) or to be pure virtual (force the subclasses to provide
the method).

And yet, I still needed a way for the subclasses to say "and also check the
stuff from the parent class".

So that's how I ended up with a pure virtual mergeable() in the base class,
and a separate method called base_mergeable().

Actually, it's possible to provide a base class implementation of a pure
virtual function for exactly the purpose of forcing an override while
providing some default behaviour (how obtuse!). The following compiles:

struct A
{
virtual int foo() = 0;
};

int A::foo()
{
return 42;
}

struct B : public A
{
virtual int foo()
{
return A::foo();
}
};

I'm not sure I'd claim this is any more elegant ;-)


One alternative which might be nicer (but OTOH not fully overridable)
is to keep the two-method design but invert it - have the base closure
mergeable() call the child class version; something like

class ClosurePrimitive {
public:
bool mergeable (const ClosurePrimitive *other) const;
{
bool base_mergeable = /* stuff from current base_mergeable () */;
return base_mergeable && derived_mergeable(other);
};

//...

protected:
virtual bool derived_mergeable (const ClosurePrimitive *other) const = 0;
};


that way the child classes can't forget to call the base_mergeable() logic.

~Chris

I second Chris's suggestion of the "inverted" two method setup.

http://codereview.appspot.com/766041/show

http://codereview.appspot.com/766041/diff/1/7
File src/liboslexec/bsdf_diffuse.cpp (right):

http://codereview.appspot.com/766041/diff/1/7#newcode51
src/liboslexec/bsdf_diffuse.cpp:51: return m_N == comp->m_N &&
base_mergeable(other);
What about the sidedness parameter in the BSDFClosure class? It doesn't
seem to be checked.

http://codereview.appspot.com/766041/diff/1/3
File src/liboslexec/closure.cpp (right):

http://codereview.appspot.com/766041/diff/1/3#newcode154
src/liboslexec/closure.cpp:154: c.weight *= w;
It is still possible to end up with 0 weights here:

Ci = diffuse(N);
Ci *= color(1, 0, 0);
Ci *= color(0, 1, 0);

(not sure if this is a problem, but we should remember the code doesn't
guarantee the weights to be non-zero)

http://codereview.appspot.com/766041/show

Just to give another alternative, leave it as it is but make
base_mergeable virtual so we allow for chained method call on the one
too. So ClosurePrimitive would have one and BSDFClosure another (doing
the sidedness check) that would in addition call the parent.

http://codereview.appspot.com/766041/show