Simplify component construction

[jonnenauha]

Recently there was some changes that made it possible to create components with a null scene to the constructor. I think before it we didn't have this problem and they were always created to a owning scene. This was a good change, we no longer need a dummy scene to put stuff into for inspection etc. Afaik this feature is mostly used when you drag and drop a .txml into Tundra and want a UI preview without actually adding it to the active scene yet. (This is what I know happens, not sure if it was the reasoning to allow creating components this way).

Anyhow, now we have a slight problem that has been going on for a while. Components that are not being maintained that well or by programmer error are crashing on this kind of creation. The feature is rarely used so it goes unnoticed untill someone does a .txml import like this.

If you use the ctor given Scene* or Framework* in the ctor youll get a crash like this. We have pretty much fixed all current components (just fixed couple ones in core and meshmoon today), but this makes writing components hard. Having to know to check for null ptrs.

Also sometimes choosing the logic to be done in ctor or handing it during component lifetime and on the ParentEntitySet signal is tricky. It is not well known to even me if ParentEntitySet signal is emitted before AttributeChanged() is invoked for the first time with the initial attribute values. So lets say that you want to make a AssetRefListener that gets used in AttributeChanged, but you only want to do that allocation if you are on a non-headless Tundra instance. As the framework cannot be reliably used in the ctor, you move the headless check and allocation into a slot connected to ParentEntitySet signal. Now, is your ref listener still null when you are handling the first value of your asset ref attribute in AttributeChanged()? Or maybe I want to do certain allocations only on a server instance?

This kind of confusion makes it harder to write components. If I cant figure out the order quickly, I would think more novice Tundra users will have immense problems following the initialization logic/order of a component.

Hence I propose that all component ctors should get a valid Framework* into them, that should be passed to the IComponent ctor and be set to the framework (currently set from the scene ptr, and left null if that is null) member variable. This way only thing that you need to be aware is if the scene is null. This change should not break anything as nothing but the component factories "new" up components. Framework could be passed to the factories to be used on component creation.

We should also document if the ParentEntitySet signal is guaranteed to be emitted before AttributesChanged() is invoked.

(Another option suggested by @Stinkfist0 would be to have Initialize() function you can override that will get called once when everything is in order.)

TL;DR We need to change so that framework ptr is always valid in component constructors.

[jonnenauha]

Case and point in EC_Mesh https://github.com/realXtend/tundra/blob/tundra2/src/Core/OgreRenderingModule/EC_Mesh.cpp?source=cc#L64-L73

world_ is null as scene is null if created unparented. Meaning the AssetRefListeners are never connected to the slots handling the asset loading. Also the ParentEntitySet signal is never connected and in that handler another vital signal is never connected.

Even worse the adjustmentNode_ is never created and that ptr is blindly used all around the source file.

This is one of our most used components and it has these glaring bugs due to the quick fixes to get it not to crash done after unparented creation was added. I'm sure I could find other examples and confusion situations but this should illustrate why we need to make this change :)

[cadaver]

Good points. I would argue though that to be clean, a component without a scene should have no business accessing the Framework; it's just a data container. Connecting signals and other "passive" activity should be OK though.

Component initialization is unnecessarily complex because I remember we started with the idea that a component can, once created, be moved or even duplicated between entities. However there should only be two cases:

1) component is created into an entity in a scene. It will never be moved to another entity or scene, but its parent entity will be set null when entity is destroyed, in case it manages to outlive the entity due to references elsewhere 2) component is created free-standing for inspection. In this case it shouldn't be moved later into an actual entity.

It should be documented that ParentEntitySet() is emitted before attribute changes.

As a radical change I would suggest even removing the scene pointer from constructor. This would make it clear that only after ParentEntitySet() the component is actually usable as something else than a data container, as only then Framework, Scene, Entity are accessible. This would make cases 1) and 2) equal from constructor's point of view.

[jonnenauha]

That works too, removing everything from the ctor would make it surely clear as well :) I wonder if we could think of a nicer name than ParentEntitySet for the signal. What this really means is "run", "go", "start" operation. Surely its what is actually going on, the parent entity is set before the signal.

Oh well... :)