Remove const_cast and simiplify linking functions

[allejok96]

I did comments to make it easier to follow along when you read the diff

[allejok96]

Reverted to be similar to old behavior. setValue only saves time if there are linked models. If not, it makes it slower. Since links are not currently saved, the likelihood of people using them right now is low. So let's optimize for the most common use, for the time being.

[szeli1]

I disagree with this change. This is a step backwards to the old and not good design. Multiple connections aren't accounted for both in yours and in mine.

[allejok96]

Your goal is to make things faster. By calling setValue you are making controllers slower, because they have to write the value to m_value, then emit dataChanged() and then return the value, instead of just returning it directly. Controllers will need to be optimized some way, but until we have agreed how I don't think you should change their behavior, especially not for the worse. Moreover, the PR is about model linking, not controllers.

[szeli1]

Currently performance is not that important. What is important is writing code that has the right structure going forward. "Optimize for maintainability, not performance" In the future this code will be changed anyways, so guiding this change is more important than performance gains.

[allejok96]

Sigh. But have the devs agreed that this is the right structure going forward? I feel there's quite some opposition to the idea of using model linking as the base for controllers. I don't think we should start changing how controllers work, until there is an agreement on where we're going.

For the time being this is a change for the worse both in terms of performance (emit) and in terms of design principles (const_cast).

There's no need to introduce this change in the circular model linking refactor.

[szeli1]

I feel there's quite some opposition to the idea of using model linking as the base for controllers.

I can't argue with this. We should have a vote.

For the time being this is a change for the worse both in terms of performance (emit) and in terms of design principles (const_cast).

Emit happened in the old code, current emit is more optimized and emit needs to happen to update the gui. In the past dataChanged() signals were connected to each other, so if one model was updated all of the linked models were updated as a result. Emit is what we want. (thinking about this, I'm unsure how your solution works without emits, maybe I'm wrong)

design principles (const_cast)

I feel like it is clear that going forward, controllers should do the updating. Most of the devs agreed with this. The const cast design while not ideal, solves the problem for now + in some of the cases also more efficient, value() will only loop trough linked knobs once while your solution loops trough every time. On average your solution will have to loop trough (n/2)*n times the linked list (where n is the number of models connected) while my solution will loop trough n times. Not to mention loading in memory this many times, this is the design flaw we want to avoid. If somebody would want to do the most correct solution, they would setValue() in the controllers. Currently we can't have this, however we can "substitute" it with the const cast. I can't see a practical problem with this approach, although the code is bad (and temporary).

[szeli1]

Also even if it works without emit, they should still emit, update() doesn't redraw the widget immediately, so even if the gui was updated with your code, multiple update()s will likely won't harm the performance.

[allejok96]

Emit happened in the old code, current emit is more optimized

There was no emit call in value() when a controller was connected. The emit to update the GUI happens in Controller::triggerFrameCounter(). But like you said, multiple calls to update() probably doesn't have a huge performance impact.

On average your solution will have to loop trough (n/2)*n times the linked list while my solution will loop trough n times

valueBuffer() still loops through the whole linked list though. And looking at it again, I just realized it doesn't work for linked models where there's no controller...

I can't see a practical problem with this approach

There's one big issue. The return value is dependent on the order of which models are checked. If model A is linked to model B and model B has a controller, checking model A's value first will return the old value, before model B gets a chance to setValue().

going forward, controllers should do the updating

I agree wholeheartedly. And I think this is the thing we should focus on, and not worry too much about optimizing value read for models that have been linked to another model with a controller, since that's very rare at the moment.

[szeli1]

There's one big issue. The return value is dependent on the order of which models are checked. If model A is linked to model B and model B has a controller, checking model A's value first will return the old value, before model B gets a chance to setValue().

This is your biggest argument. Add a comment about the value() design flaw and how it would be ideal to have the most up to date value for our models stored in m_value. I will merge afterwards.

[allejok96]

looking at it again, I just realized it doesn't work for linked models where there's no controller...

I was wrong about this. It works, it's just calculated independently (but identically) in each linked model's valueBuffer. I added a comment about that too.

[szeli1]

Thanks for making these changes. The for loops instead of while improved the code.

Please react to my suggestions.

[szeli1]

I disagree with this change. This is a step backwards to the old and not good design. Multiple connections aren't accounted for both in yours and in mine.

[allejok96]

I saw messmerds review. What's the status here?