Bugfix ofParameter change name

[eduardfrigola]

As discussed in #6576 and PlaymodesStudio#1 this PR fixes the issue when a parameter changed it's name.
It adds an event dispatched by ofParameter when the name is changed to its parents, which can also be captured externally.
Also adds implementations for ofxGui capturing those events and updating the gui accordingly.
cc @roymacdonald

[eduardfrigola]

I found an issue with what we had.
As ofParameterGroup uses map to store parameter names, we cannot use the same name for two parameters, so we need to allow / block name changes.
For this reason setName now returns a bool instead of not returning anything, and tells you if the name is changed. This is a different way of dealing with duplicated names, as ofParameterGroup::add only warns you of this

openFrameworks/libs/openFrameworks/types/ofParameterGroup.cpp

Lines 15 to 17 in 51bcbf3

	if(obj->parametersIndex.find(name) != obj->parametersIndex.end()){
	ofLogWarning() << "Adding another parameter with same name '" << param->getName() << "' to group '" << getName() << "'";
	}

Maybe that is something that should be done outside this? First checking by yourself if the name exists in all of it's parents? I think that, as the error you create is irreversible, this should not be allowed, and notified accordingly.

[roymacdonald]

Thanks @eduardfrigola for bringing this issue back.
What if we create unique names for each parameter stored in the group? I have not seen for a while the code for ofParameterGroup so I am not totally sure if it might work, but definitely we need a better solution for this.

[eduardfrigola]

As we only use the name for identifying parameters inside the group, if we want unique names, either only unique names are allowed, or ofParameterGroup adds something to the name of the parameter (modifying the original parameter) to be unique.

Maybe this is something we need to discuss with @arturoc who created ofParameterGroup...

[artificiel]

this is a design problem as for many use cases, a group may hold multiple parameters with the same name... but in the context of ofxGui it is confusing, and in the context of getting parameters by name it is downright problematic.

so the design needs adjustment. if an accessor approach was used with []or .at(), such as group["my param"] = my_param, it makes double-presence impossible, and intuitively understood as such.

.add() could be deprecated, and modified in such as .add(my_param) would replace a previously existing my_param, keeping things sane. that will break existing code relying on the presence of identically named params -- but it's easily arguable that an ofParameterGroup with identically named members is a bug.

unfortunately, the ofParameterGroup API being mainly reference-based, group["my param"] = my_param cannot work as group["my_param"] returns a reference. i understand the appeal of references (especially in pre-shared_ptr designs, and facilite dot syntax) but in order for this to work it would need to be switched to pointers — NB it's already shared_ptr within the implementation, and there is little to no usage of the group["name"] idiom anywhere, so it could be manageable.

[eduardfrigola]

Hi @artificiel, thanks for taking a look at this.
This PR was proposed as a fix for #6576, trying to fix the ofParameterGroup::contains() we maybe got the wrong path here...
I understand that you point is that ofParameterGroup should allow having more than one parameter with the same name, those parameters would be two different instances, and the reference (or shared pointer #8128) should be sufficient for the group not to confuse both.

As also stated in the issue @6576 ofParameter::add warns you about name duplication but does allow you to do that. So changing a name of a parameter inside the group is something that should also be allowed and just warned right?

I initially wanted to fix the issue from another approach #6577 (fixing the same as this PR) changing the way ofParameterGroup access it's elements, but we had a performance loss and was not an option.

Just for a note, I have been using this PR in my fork and it has not caused me any issues.

Let's see which way we can address this.
If you prefer, we can continue discussion in #6576 and find a better approach for that issue.

Eduard

[artificiel]

@eduardfrigola ah sorry if i'm a bit off-topic; to be clear i think this PR's name change propagation idea is great. (i ended up here while checking your branch's commits because it's unfortunate that an actively developed addon like ofxOceanode requires OF 11.2).

so i was reacting/expanding to your mention of:

As ofParameterGroup uses map to store parameter names, we cannot use the same name for two parameters

which (vector for param and map for names) is a design problem for plain usage too, and unique name enforcement should be wider that just on checking changes, as it does not make much sense to have multiple params with the same name in a group if the name is to be relied upon.

implying the uniqueness of names with an accessor interface like .at() or [] (instead of an accumulator like .add()) makes it explicit that you will be replacing any previous associations to the same name.

but that does not change your PR's core solution of what happens if someone changes the param's name later. i haven't run this PR (it unfortunately no longer auto merges) but i presume the usage expected to be:

ofParameter<float> f1 {"f1",1}, f2 {"f2",2};
ofParameterGroup group {"group", g, f};
if (!f1.setName("f2")) {
  // name change rejected because of a sibling in group
  // f1 is still named f1
} else {
  // f1 now named f2
}

i guess where i'm going is that maybe ofParameterGroup should have an enforceUniqueNames setting that would be more restrictive (for example in ofParameterGroup::add() properly replace an existing name like .at()/[] would do instead of keeping the previous one but now "unaccessible" (and buggy in ofxGui). exceptions made for anonymous params (anyway otherwise anonymous from the user's perspective). to be clear whay i mean by anonymous is like:

ofParameter<float> f2 {1}, g2 {2};
ofParameterGroup group2 {"group2", g2, f2};

which is a perfectly fine use (obviously not meant for ofxGui nor to retrieve with a name) but still generates Adding another parameter with same name ''... so an empty name param would be excluded from unique name enforcement.

so yeah, not strictly on topic with this PR in terms of "code review" but related in how names should be handled and presented to the user.

[eduardfrigola]

Hi @artificiel

Your idea of having an enforceUniqueNames setting seems a good solution to try to fix this issue without breaking current functionality (as generating params without a name)

One other thing that might be a fix is to initialise ofParameter with a uuid if no name is set, so ofParameterGroup could store the reference of the parameter correctly. Maybe empty names is the only place where parameters with the same name make sense...
For the use of ofParameterGroup (mainly with gui stuff) having unique names is the way to go.

Storing parameters as references could make the issue about names disappear (with enforceUniqueNames to false). But a lot of ofParameterGroup functionality is invalid for this case (all the getIntParameter(), getBoolParameter(), contains()).

If you want to test this proposed solution, you can checkout playmodesstudio/openframeworks:master, which has been been recently merged with the latest changes in master. There are changes in the unit test for ofParameter that check for changing the name after putting into the group. And the behaviour is exactly what you presumed.
Here there is an exampled currently in use with ofxOceanode:
https://github.com/PlaymodesStudio/ofxOceanode/blob/b3bab16995f9b85317a934d865b8e5ad2012488b/src/Managers/ofxOceanodeTypesRegistry.h#L82-L86

Eduard