Rewrite all components to use the new optimized IComponent::AttributesChanged() message.

[juj]

To avoid performance issues with multiple batched changes to the attributes of a component, the message IComponent::AttributesChanged() was implemented in this commit: https://github.com/realXtend/naali/commit/226ed4cca80935c778d86b3439cd7014c1e62143 .

The new form allows the component to avoid doing potentially costly updates multiple times. The EC_HoveringText is a good example of the new system. Also, EC_Terrain uses the new message.

The use of signal IComponent::AttributeChanged(IAttribute* attribute, AttributeChange::Type change) should be removed altogether from components internally.

[cvetan5]

Does this message also handles the update mode like in AttributeChanged(IAttribute*, AttributeChange) signal? Talking about AttributeChange::Disconnected, ::LocalOnly, ::Replicated things. For example, EC_SkyX uses these more than any other component https://github.com/realXtend/naali/blob/tundra2/src/Application/SkyXHydrax/EC_SkyX.cpp#L403

[jonnenauha]

Leave those alone that really use the change enum. Those cant be ported and prolly need to use this old signal to get that information.

[Stinkfist0]

Note that EC_Sky is already taken care of in this commit https://github.com/LudoCraft/Tundra/commit/9c45ee232f2fba2a6cf52230f340e225c895a3ea

[Stinkfist0]

Coming to think of it, this issue would be probably best solved without conflicts after Tundra2 lc and TundraCore pull requests are merged to realxtend/naali.

[jonnenauha]

Does it move a lot of components then? I don't see those causing major problems if they don't move around the comp files. This is done on a feature/bugfix branch against rex/tundra2. Can be merged later if there are other pull requests that get in first.

[antont]

I'm a bit puzzled by the use of the new signal for example in that SkyX case for material changes:

void EC_Sky::AttributesChanged() { if (materialRef.ValueChanged()) ...

previously the signal itself told what attribute(s) change, right -- isn't that better for handing the changes, instead of checking from the data like that? I understood that the new solution solves the issue of the same signal being emitted multiple times and being then handled many times in implementing code. But isn't this tech / workaround quite a hack, do we already know better options or is that change really the design we wanna go with?

[Stinkfist0]

@antont Note that you mixing Sky and SkyX in your comment. To clear possible confusion, there is no new signal per se, AttributesChanged is a virtual function that ECs can implement if wanted.

[cvetan5]

@Stinkfist0 I will leave alone EC_SkyX in that case. I touched all other components though, but mostly it is OnAttributeChanged(IAttribute...) to AttributesChanged() and if (attr == &someAttr) to if (someAttr.ValueChanged()) but that does not mean there won't be any conflicts

[jonnenauha]

@antont The point is to fire ComponentChanged signal once and check what attrb actually changed with one single run of the function, when all of the attributes are set. Previously we would fire the signal after each individual change. So afaik this is an optimization and really reduces the signals and runs through the handler function when we talk about 500 entities with x amount of components when the scene replicated over to the client :)

I might be wrong but this is my understanding of it. Also you can order you handler function to process attributes in a certain order once something is changed, before I think it was pretty much random what attribute change signal fired and it was harder to track in what state the component was etc.

@cvetan5 Yeah that change might affect in the order of thing. If before the signals fired with "component index" or the "order they came from network" and now the order is exactly in what order the function handles the attributes. This can cause big problems in some components im afraid :I The init order has one time to get it right when all things have changed.

[Stinkfist0]

@cvetan5 I have touched EC_Sky, not EC_SkyX.

[cvetan5]

@Stinkfist0 oh right, sorry. Reverting EC_Sky then

[juj]

In the old form we had the changed attr as parameter, and the flow was to do

if (changedAttr == text) x else if (changedAttr == font) x else if (changedAttr == fontSize) x .. ..

the new form doesn't have this member, and the flow is

if (text.ValueChanged()) x if (font.ValueChanged()) x if fontSize.ValueChanged) x

so there isn't a functional difference there that is more or less hackish than before. We still do a O(n) test to see what changed.

The functional improvement is the way batching now occurs, and now everything is grouped, and we can say this:

https://github.com/realXtend/naali/commit/226ed4cca80935c778d86b3439cd7014c1e62143#L4R351

in the old form, EC_HoveringText had to do shadowed double-buffering of each attr value, or call ShowMessage() once for each changed attr, and there's 14 of them! Indeed in the old code, the text was repainted 14 times when a new EC_HoveringText component was created.

Avoiding Qt signals from being fired was not a motivation for this optimization. They still are fired. And probably will have to be, so that code external to the component implementation can listen to value changes. The only optimization here is to allow each component to react to changes in batches in a natural way without having to shadow-copy values.

[juj]

@cvetan5: the new form of AttributesChanged() triggers on all change types. If that's not good for the component, use the old handler form for those variables.

[jonnenauha]

Still the amount of signal trigger to slot call will drop drastically when this is done across the board for all ECs. The impact can be "measurable" if you have thousands of entities with N components, so maybe it's a micro optimization in that sense :)

[Stinkfist0]

@jonnenauha @cvetan5 In TundraCore EC_Sky, EC_Fog, EC_EnvironmentLight, EC_Billboard and EC_ParticleSystem are relocated to OgreRenderingModule.

[jonnenauha]

@cvetan5 Should this be closed? Or did you leave out some of the components on purpose?

[cvetan5]

The components that were left out were only so because the old handler was better for them. If no one else is having problems, this can be closed.