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.