ofxAssimp::Bone inheritance issue

[dimitre]

Bone is derived from class ofxAssimp::Node that derives from ofNode. ofNode has a virtual void draw() const; function with no parameters

and in Bone we have

	virtual void draw(float aAxisSize=30.0f);

so it hides draw from ofNode, but in doxygen we can see a message not to override. which is strange. if it is not meant to override why virtual ?

	/// \brief Draw the node as a white cube with xyz axes.
	/// \note do NOT override this.
	/// It transforms the node to its position+orientation+scale
	/// and calls the virtual 'customDraw' method above which you CAN override.
	virtual void draw() const;

so I tried to mark as final and it shows it is overriden in of3dPrimitive class.

we should not use virtual when not meant to be virtual override always when overriding virtual and final IF there is a case that is really not meant to be overriden

[dimitre]

Same issue is present in ofxSvg. in its inheritance ofxSvg -> ofxSvgGroup -> ofxSvgElement -> ofNode we have virtual void draw() const override; on ofxSvgGroup, not present in ofxSvgElement and then present again in ofNode.

[dimitre]

ofLight uses it to draw lights of3dPrimitive uses in a very strange way. it tries to import using

using ofNode::draw;

and soon it replaces with

void draw() const;

my suggestion is fix everything by renaming ofNode::draw const to ofNode::drawNode, adjust in ofLight And have draw in ofxSvg / ofxAssimp not inherited from ofNode, or inherited by adding ofNode::draw (non const)

[ofTheo]

Good catch @dimitre!

I think in practice customDraw might not be as used as draw, despite the instructions :) We should find a good solution to this.

I don't think we can rename ofNode::draw unfortunately. As a class that is designed to be inherited it is used by quite a few projects and addons. So would definitely break.

Search results for "public ofNode"

[dimitre]

Nice. Let's look which of this classes uses primitive ofNode::draw so we know if there is any meaningful break.

On the top of that ofNode has a pretty important destructor (removing listeners) and I think it should be called from all other desctructors too we can do that by setting something like:

//----------------------------------------------------------
of3dPrimitive::~of3dPrimitive() {
	ofNode::~ofNode();
}

and the same for all other derived classes

[NickHardeman]

@dimitre The destructor for ofNode gets called without having to explicitly call it from a derived class as long as the destructor(s) are virtual. A simple example using ofPlanePrimitive.

ofPlanePrimitive::~ofPlanePrimitive() {ofLogNotice("ofPlanePrimitive::~ofPlanePrimitive");}
of3dPrimitive::~of3dPrimitive() {
	ofLogNotice("of3dPrimitive::~of3dPrimitive()");
}
// same for ofNode

{
	ofLogNotice("ofApp") << "Going to call plane constructor";
	ofPlanePrimitive plane;
	ofLogNotice("ofApp") << "plane going out of scope";
}
ofLogNotice("ofApp") << "plane out of scope";

Outputs the following

[notice ] ofApp: Going to call plane constructor
[notice ] ofApp: plane going out of scope
[notice ] ofPlanePrimitive::~ofPlanePrimitive: 
[notice ] of3dPrimitive::~of3dPrimitive(): 
[notice ] ofNode::~ofNode: 
[notice ] ofApp: plane out of scope