ofParameterGroup contains(string name) if parameter changed name.

[eduardfrigola]

Hi everyone.
Playing with ofParameterGroups I found an issue with ofParameterGroup::contains(...)

openFrameworks/libs/openFrameworks/types/ofParameterGroup.cpp

Lines 422 to 424 in 5453d35

	bool ofParameterGroup::contains(const string& name) const{
	return obj->parametersIndex.find(escape(name))!=obj->parametersIndex.end();
	}

it also applies in all places where std::map<std::string,std::size_t> parametersIndex; is used.

The issue pops up when you change the name of a parameter that is already inside the parameterGroup. The name changes of the parameter changes, but the name in parametersIndex does, not and so we cannot find our parameter inside the parameterGroup. Because the parameterIndex map is only created when adding parameters, and only modified when removing, the only workaround for this is to remove the element from the group, changing the name, and adding it again. With that changing the order of the elements in the group.

As ofParameter has a public method of ::setName(), this case should be considered right?

As there is no event fired by the ofParameter when the name is changed, I think the best solution is to get rid of parametersIndex and search the parameters vector directly so we get the updated name every time we make the request.

Eduard

[roymacdonald]

Hey @eduardfrigola
It makes sense to me to remove anything that is duplicated data, as it is in this case.
Although, I guess that parametersIndex was added to improve performance. So, what about testing how performance is afected by removing parametersIndex ? Otherwise, having an ofEvent triggered when the name changes can be another option, that can be useful on some other scenarios.

[eduardfrigola]

Hi @roymacdonald thanks for answering.
I made test with bundled OF unittests for ofParameter and didn't notice any notable difference... But the test is with small amount of parameters inside the group, so I will make a test adding a ton of parameters.
Your proposal of a change name event will be a better option, for example, for detecting if the group already has a parameter with that name on it, as it does when you add() a parameter.

Maybe handling name changes once added to parameterGroup it is not a thing to be allowed, since probably you passed that group to somewhere else (ofxGui) and modifying that causes some errors. Have you encountered something similar?

[roymacdonald]

Hi, let me know how it goes with those tests.
ofxGui uses the ofParameter's name. I have not seen errors related to name changes, and I assume it is not problematic from checking the source code. Have you seen problems of this kind?

[eduardfrigola]

Hi Roy,

So I took the time to made the tests. I added 0, 20, 100, and 10000 parameters to the group in the existing test. I did not change the original code, where 4 parameters are added, and 1 is removed. so that's why there are this strange number of parameters.
I've ran each combination 5 times and made the average (the results where very similar on each try). I changed default millisecond resolution in unit tests with microsecond resolution.

Values in Microseconds	patch-release branch	bugfix-parametersIndex branch
3 Parameters	191,6 μs	197,4 μs
23 Parameters	257,6 μs	286,4 μs
103 Parameters	392,8 μs	1059,2 μs
10003 Parameters	21745,4 μs	6416707 μs

So from the result I read that, for most cases (3 to 23 parameters in the group), there is no noticeable difference. But as you have more parameters the difference grows exponentially (or seems to).
Then, making this change that could slow down the performance, and that there is no possible way to recover the previous performance, seems to be not a valid option...

For me this problem arose in the development of ofxOceanode, a visual patching addon that uses internally ofParameter. This issue in particular I found it now, when trying to update the Macros (similar in concept of a patcher in max/msp). The problem is that I want to be able to change the name of the parameter that is added to the node (Green node in the GIF) that I created with the dark blue node (inside the Macro), where I want to be able to change the name of the parameter and therefore the name in the green node GUI. I'm using ImGui in this case, not ofxGui by the way.

I found some similar issues with adding scripting functionality, and for now what I did was to destroy the already created parameter and create a new one with the new name. But that reorders the parameters in the group and also loses the state of that parameter (things like connections, midi bindings, and other stuff).

Sorry for not being clear, but I find it hard to explain with a few words. Saying that, I also feel that I have such a special case that maybe ofParameterGroup is not well suited for what I'm trying to achieve, and I have to make a custom container for my parameters.

I hope that clarifies something, and why I see that as an issue.

Thanks
Eduard

[roymacdonald]

Hey @eduardfrigola
I totally understand what you mean. Thanks for doing those tests. You are right, most use cases will not use more than a 100 parameters so it seems not that terrible.

Although I think that the proper solution would be to add an event for when the ofAbstractParameter (the base class of ofParameter and ofParameterGroup) changes its name, this way the parent ofParameterGroup will listen for the event and change parametersIndex accordingly. I can make those changes.

Btw, I really like what you are doing with ofxOceanNode, its really cool. I've been fiddling with it several times. (and I really like the work you do with Playmodes as well :) )
I just published this addon https://github.com/roymacdonald/ofxLineaDeTiempo which might be useful with ofxOceanode, as it also relies on ofParameters to work, so in theory these should be compatible.

[eduardfrigola]

I agree that would be a better solution. Let me now if I can help you with that!

Thanks for your kind words Roy! I tried LineaDeTimpo the day you posted! Feels very nice! Timeline to ofxOceanode it's the next thing to add once we have Imgui fully working!

[roymacdonald]

Sure, I'll let you know. I guess that testing it with your setup would help.

I'll write you an email regarding ofxLineaDeTiempo and ofxOceanode.

[eduardfrigola]

Hi Roy!

I've tried your idea about notifying parameterGroup when the parameter has it's name changed. It does not add a lot of code, and it works fine for my scenario, and for some tests I've ran. I don't know if in some strange situations it may break...
You can check it at: https://github.com/PlaymodesStudio/openFrameworks/tree/bugfix-ofParameter_change_name

Let's see if we can get this fixed!

[roymacdonald]

Hey @eduardfrigola Looks good to me, yet, wouldn't it be better to have a function in which the name changes is notified instead of having 2 identical blocks of code copied? I'll give it a try tomorrow and test it. I'll let you know

[eduardfrigola]

Sure, a function would be cleaner, let me know if you need something!

[roymacdonald]

@eduardfrigola I was looking at this and I think that the following makes sense:

• Make a function, maybe in the ofAbstractParameter class for the following block

  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());
  

  as parents belong to each subclass of ofAbstractParameter it should be a function which receives the parents object and performs the check.

• for all ofAbstractParameter subclasses setName(...) implementation perform this parents check and notify the name change.

• Only notify the name change if oldName is not empty, otherwise it kinda does not make sense

What do you think? Can you make this changes or do you want me to do them?

best

[eduardfrigola]

Hi @roymacdonald,
So the block you say is found in other functions too, such as ofParameter::eventsSetValue() and ofParameter::trigger()

The second one you will see has also similar code duplicated another time, it was me, in the same situation as now. If we can go for the same path for removing both code duplication I think will be more understandable.

One option that maybe confuses more than anything would be to make setName non virtual in ofAbstractParameter, and implement the notification to parents, and in this function also call a new virtual function as setObjectName, where the subclases will override with what they are now overriding setName. It is a way to avoid code duplication, but maybe it is not the proper way to do that design-wise.

The later thing you say about check if it is not empty it's totally true, and that also maybe should be checked elsewhere, as in ofParameterGroup::add(...)?

Eduard

[roymacdonald]

Hey @eduardfrigola
Yes, that block of code is elsewhere so it would be a nice cleanup to make it a function.

One option that maybe confuses more than anything would be to make setName non virtual in ofAbstractParameter, and implement the notification to parents, and in this function also call a new virtual function as setObjectName, where the subclases will override with what they are now overriding setName. It is a way to avoid code duplication, but maybe it is not the proper way to do that design-wise.

Right, that could be a way to go but it could be done simply with an auxiliary function, as not all the ofAbstractParameter subclasses would do the same thing. Also, the data belongs to the subclasses, which wont be accessible from the base class. I've coded something that's working. I'll post it soon