ofParameter<void> notification loop

[eduardfrigola]

Hello everyone.

I'm in a situation where I create ofParameters with .:newReference() function.
With that I have both ofParameters "linked together" similar to @arturoc ofParameterLink proposal

I have no issues with that except with ofParameter, as there is no notification loop safeguard.
I've resolved this issue updating what I did in this change with the notifiy loop safeguard implemented in :

openFrameworks/libs/openFrameworks/types/ofParameter.h

Lines 726 to 766 in b674f7e

	inline void ofParameter<ParameterType>::eventsSetValue(const ParameterType & v){
	// If the object is notifying its parents, just set the value without triggering an event.
	if(obj->bInNotify)
	{
	noEventsSetValue(v);
	}
	else
	{
	// Mark the object as in its notification loop.
	obj->bInNotify = true;
	// Set the value.
	obj->value = v;
	// Notify any local subscribers.
	ofNotifyEvent(obj->changedE,obj->value,this);
	// Notify all parents, if there are any.
	if(!obj->parents.empty())
	{
	// Erase each invalid parent
	obj->parents.erase(std::remove_if(obj->parents.begin(),
	obj->parents.end(),
	[this](const std::weak_ptr<ofParameterGroup::Value> & p){ return p.expired(); }),
	obj->parents.end());
	// notify all leftover (valid) parents of this object's changed value.
	// this can't happen in the same iterator as above, because a notified listener
	// might perform similar cleanups that would corrupt our iterator
	// (which appens for example if the listener calls getFirstParent on us)
	for(auto & parent: obj->parents){
	auto p = parent.lock();
	if(p){
	p->notifyParameterChanged(*this);
	}
	}
	}
	obj->bInNotify = false;
	}
	}

See implementation in: PlaymodesStudio@7bbf058

My doubt is about using this approach, or using the setMethod / eventsSetValue / noEventsSetValue. Seems as a more elegant solution but, as ofParameter has no value, its use is only for triggering events noEventsSetValue is not a needed function.

What do you think about that?

Thanks!
Eduard

[ofTheo]

@eduardfrigola thanks for this!

Would you be able to post the most minimal working ofApp example that would produce a bug/recursive loop with the current patch-release branch? I think seeing the issue in code would be helpful.

Can you see any possible problems / bugs / regressions with the fix you describe above?

Thanks!
Theo

[eduardfrigola]

hi Theo, now I see that this issue is also happening prior to #6471, I checked 0.11.0 tag.

The example I present compares behaviour of ofParameter and ofParameter, as an example does not seem to have a lot of sense. But using it in a real world scenario where you want to control the same thing in different places, linking parameters is a simple way to do so. Maybe it is not the most elegant solution, but as ofParameter (and all other templated types) has the safeguard to make this possible, I think ofParameter should have it too.

// ofApp.h
...
ofParameter<void> void1;
ofParameter<void> void2;

ofParameter<float> float1;
ofParameter<float> float2;

ofEventListeners listeners;
...

// ofApp.cpp
...
void ofApp::setup(){
    
    listeners.push(void1.newListener([this](){
        void2.trigger();
        ofLog() << "Void1 event listener";
    }));

    listeners.push(void2.newListener([this](){
        void1.trigger();
        ofLog() << "Void2 event listener";
    }));

    listeners.push(float1.newListener([this](float &f){
        float2 = f;
        ofLog() << "Float2 event listener";
    }));

    listeners.push(float2.newListener([this](float &f){
        float1 = f;
        ofLog() << "Float1 event listener";
    }));
    float1 = 1;
    void1.trigger();
}
...

Here you can see that float puts the value in the two "linked parameters" (float1, and float2) but void enters in a infinite loop.

You can checkout this branch to try the expected behaviour (I can make a pull request if that is easier for you to test).

The problem I see with my proposal is about the diference with the templated parameter, where a more complex approach is used with eventsSetValue / noEventsSetValue. Also the code duplication is not a very good strategy, isn't it? could trigger() just call trigger(this)?

Eduard

[ofTheo]

Thanks for this Eduard.
It is really helpful to see this to understand the issue.

My first impression is that this is a pretty specific use case and def feels more like a feature beyond the intended use case rather than a bug or regression.

I think it might be good to have others have a look at the proposed changes as it is something I have not had a need for, but others might have:
PlaymodesStudio@7bbf058

I think the advanced ofParameter PR from @arturoc you mention above is a little easier to understand the actual usage.

Will mark this for 0.12 and add to the 0.12 list though, so we can make sure to resolve it.

Thanks again!

[eduardfrigola]

Yes, I'm working pretty hard with ofParameters and finding issues / needs that sure are beyond the intended use case, and maybe for 0.12 we could discuss those.

For this specific issue, do wherever you thing it's safer for 0.11.1 release.

[roymacdonald]

@eduardfrigola can you make a PR of this? I dont seem to find it.

[eduardfrigola]

Hi Roy, sorry I left this unattended. I took the time to rebase the changes I made, and perform some more tests to be sure it still fixes the issue described here.

[eduardfrigola]

Hi all.

I noticed this issue is assigned to the 0.12.1 milestone.
Should it be addressed before releasing 0.12.1?

Revisiting the solution in PR #7122, it seems that fixing this issue would simply align the behavior of ofParameter<bool> with ofParameter<T>. Since this is indeed a bug that others might encounter, I believe ofParameter<bool> should not behave differently from other ofParameter types.

That said, since no one else has reported this issue in over four years (though it remains a blocker for me and one reason why ofxOceanode is not compatible with the latest openFrameworks versions), we could also consider postponing it to 0.13.0. That way, we can discuss it along with other ofParameter related issues: #6576, #8128, #8127.

What do you think?