IGUIElement bug

[clarks]

In the constructor for IGUIElement the following exist

Code: Select all

 
// if we were given a parent to attach to
        if (parent)
        {
            parent->addChildToEnd(this);
            recalculateAbsolutePosition(true);
        }
 

parent->addChildToEnd(this) should be changed to addChild(this) because the latter is virtual. When elements are being added to a parent upon creation, advanced ui elements will not know because they depend on the virtual function addChild which is not being called.

[Mel]

addChild really only calls addChildToEnd, and updates the given object's position, both are implemented on the IGUIElement class, it being virtual only means that if any derived class has to reimplement the addChild method, the correct method will be called during the run time, but if not, there is still a default definition. Don't worry, this shouldn't cause any bug on a derived class.

[CuteAlien]

Actually addChild was used originally, but that was a bug. The documentation for addChildToEnd describes it already a little - we have to use a non-virtual function here because when you call virtual functions in a c++ constructor the derived function is _not_ called. Which is then even more confusing.

I made you quick example where you can try it yourself:

Code: Select all

 
#include <iostream>
 
class Base
{
public:
    Base()
    {
        Foo();
    }
    
    virtual void Foo()
    {
        std::cout << "Base\n";
    }
};
 
class Derived : public Base
{
public: 
 
    virtual void Foo()
    {
        std::cout << "Derived\n";
    }
 
};
 
int main()
{
    Derived d;
 
    return 0;
}
 

Here's a link describing this better: http://www.artima.com/cppsource/nevercall.html

A similar problem is also in ISceneNode: http://sourceforge.net/tracker/?func=de ... tid=540676

We haven't discussed yet how that should be solved. I suspect the real problem here is that adding/removing elements should not happen at all in the IGUIElement and ISceneNode classes but rather in IGUIEnvironment and ISceneManager. That might also help with other troubles like that the environment should sometimes know when an element is removed.

[clarks]

I suspect the real problem here is that adding/removing elements should not happen at all in the IGUIElement and ISceneNode classes but rather in IGUIEnvironment and ISceneManager.

I believe that this is the problem. Rather than telling the parent to add child from the childs constructor, doing it from a higher level makes more sense. This sounds like a good solution.

[randomMesh]

When my base class's constructor calls a virtual function on its this object, why doesn't my derived class's override of that virtual function get invoked?
Never Call Virtual Functions during Construction or Destruction
Joy of Programming: Calling Virtual Functions from Constructors

[CuteAlien]

@randomMesh: There's no discussion that this is wrong. The real problem as usual is how do you change stuff that is already in use since years and where everyone will hate you for breaking it as it works 99.9% and is used in their code. So you better make very, very sure that next time you do it is really, really correct because if you have to change it a second time people will hate it even more. So the simple solution I used in IGUIElement was replacing it with a non-virtual function - which still is not 100% correct as one expects the virtual addChild to be used which it simply isn't, but had the advantage that it's at least technically correct and doesn't break any interfaces. But a better long-term solution might be the one mentioned above and breaking the interface hard and moving that stuff into manager classes. But then again - when such a big interface break is necessary you don't want to do it often. So maybe a more modern engine shouldn't use that structure at all? Because especially when it comes to ISceneNodes you might have noticed that other engines by now avoid all virtual functions and pointers like hell when it comes to matrix-access and instead make certain all matrices are kept together in simply cache-friendly buffers. Does finding such a solution influence adding/removing scenenodes? Maybe - I really haven't figure that out yet - but I better should do that before breaking an interface as otherwise I risk having to break it twice in the long run.

And there is also the other problem - that it makes sense sometimes keeping those trees outside a scenemanager/gui-manager (for multiple scenes for example). So maybe current structure even is correct...

I guess we can maybe go slowly in that direction. Adding empty constructors for a start. Then using those empty construtors followed by addChild functions in the addNode functions of the managers at least.