monome / norns

norns is many sound instruments.
http://monome.org
GNU General Public License v3.0
633 stars 147 forks source link

params: align 'add_separator' and 'add_group' flow with other paramtypes #1584

Closed dndrks closed 2 years ago

dndrks commented 2 years ago

while scripting tonight, i realized https://github.com/monome/norns/pull/1539 did not anticipate situations where an author might do the following:

params:add_number('my var', 'my var', 1, 127)
[...]
params:add_separator('my var')

though it's convention to use underscores within parameter ID's, the system supports ID instantiation with spaces, so there's a chance someone will accidentally clobber their useful parameter with a separator or group name. since separators and groups do not have ID's, only names, this seems like a good guardrail.

tehn commented 2 years ago

is this the right fix? if there is a copy of the name, then it works but doesn't have an entry in the lookup? rather should it just fail and issue an error?

dndrks commented 2 years ago

for sure! i was conflicted on what would be the easiest most-right fix, honestly. all the other param types use the :add method, where these conflicts are caught, but add_separator and add_group manipulate state external to the :add method.

though i'm a little gun shy about adding any fails / error messages to the params after the issues earlier this year, i popped a few into the latest commit. full fail doesn't feel necessary, since most cases of params:lookup_param seem to be motivated by .action manipulation anyway -- having an error and no overwrite to the previous instance of the action-oriented ID when a separator or group tries to use it feels a lot more informative + actionable!

220714: i also sorta wonder if separators and groups (since their states can be scripted) should adhere to the id/name mechanism that all other parameters require. i can imagine situations where an author wouldn't likely care about separators being blocked from registration (eg. if i'm instantiating a separator named 'voice 1' across many groups of params) as well as situations where they would (eg. if i'm hoping to selectively hide/show specific 'voice 1'-named separators under certain conditions). i wonder if the option to add a unique ID to separators and groups could help clear up these cases?

dndrks commented 2 years ago

i wonder if the option to add a unique ID to separators and groups could help clear up these cases?

got to spend some time on this and it works! i ended up changing :add_separator and :add_group to use the standard :add method (like all the other parameter builders), so that ID conflicts could be accurately tallied and errors printed. this required adhering to the name and id requirement of all other paramtypes, which required placing some guardrails for expected historical invocation:

separators

groups

in all cases, the important thing is to not add the parameter lookup for a meaningful conflict, which this addresses!

example script ```lua function init() print("1 --------") -- will not error, historical use: params:add_group('test group one',1) params:add_separator('1') print('historical use will not error') print("2 --------") -- will error, as ID's are same params:add_group('name with spaces') -- if just name is provided, this becomes the ID params:add_number('name with spaces', 'name with spaces') -- ID is same as group! print(" ^ parameters with identical ID's will cause error") print("3 --------") -- will alert, as group ID's conflict: params:add_group('test group two', 1) params:add_separator('three') params:add_group('test group two', 1) params:add_separator('four') print(" ^ groups with identical names will alert because they do not have unique ID's") print("4 --------") -- will not error, group ID's provided: params:add_group('g_test_1', 'test group one',1) params:add_number('number_1a','1a', 0, 100, math.random(100)) params:add_group('g_test_2', 'test group one',1) params:add_number('number_1b','1b', 0, 100, math.random(100)) print("groups with identical names did not error because they have unique ID's") print("5 --------") -- will error, as separator id's conflict: params:add_separator('five') params:add_separator('five') print(" ^ separators with identical ID's will alert") print("6 --------") -- will not error, as separator id's are different: params:add_separator('sep_fiveA', 'five') params:add_separator('sep_fiveB', 'five') print("separators with identical names did not error because they have unique ID's") print("7 --------") -- will not error, as default 'separator' ID is skipped: params:add_separator() params:add_separator() print("separators without ID's or names are given 'separator' as an ID, but norns will not call it out as an error") print("8 --------") -- will not error, as default 'group' ID is skipped: params:add_group() params:add_separator('example_8a','hello') params:add_group() params:add_separator('example_8b','good to see you') print("groups without ID's or names are given 'group' as an ID, but norns will not call it out as an error") end ```

please feel free to lmk if this all feels aligned or if there's more to consider! i just wanna get all param types using the same basic building blocks, totally open to any redirects / feedback -- tyty!

dndrks commented 2 years ago

ok, ready for real now -- sorry for the extra traffic!

dndrks commented 2 years ago

had a quick thought, @tehn -- entering MAP mode on params surfaces the param ID for each parameter, but the current code excludes separators and groups from this changeover. do we want to retain this behavior now that groups and separators can have ID's? if only a single string is provided at instantiation (eg. historical use of passing a name only), the ID defaults to the name, so shouldn't cause any backwards compatibility issues and i could see an author using the MAP mode to double-check their parameter IDs populate correctly.

just lmk which way you feel we should go and i'll propose the small change in another PR if we should adjust!