The parameter clobbering warning compares parameter IDs with parameter separator IDs

[xmacex]

Hi.

The useful and necessary parameter clobbering warning triggers if actual parameters IDs match parameter separator IDs. I would consider this a bug in norns, on the argument that while warnings about parameter clobbering is a great service, comparing parameters with parameter separators is to compare apples with oranges¹.

    ⚖️
  ⚖️⚖️⚖️
⚖️  ⚖️  ⚖️
🍏  ⚖️  🍊
    ⚖️
⚖️⚖️⚖️⚖️⚖️

Case initenere reported on lines

!!! ERROR: parameter ID collision: scale
! please contact the script maintainer - this will cause a load failure in future updates
! BEWARE! clobbering a script or mod param

https://github.com/linusschrab/initenere/blob/main/initenere.lua#L403

If one renames the parameter separator ID to eg. "scales" or "scale params" ie something which does not clash with the actual scale parameter on line 403, the warning goes away. Undo, and it's back.

I realize that parameter groups are kinds of parameters too... maybe. Is clashing parameter and separator IDs consequential? I would suggest either being more strict in the comparison and only check within a type ("type" loosely understood) before issuing the warning, or being more specific in the error message that parameter separator should have IDs unique of the parameters and also document it in the scripting reference.

¹ I love fruit salad

Xoxo

[catfact]

Is clashing parameter and separator IDs consequential?

yes

[catfact]

sorry for minimal response. i haven't reviewed this in a while (since the warning went in.)

the warning went in actually specifically because of the addition of these "divider" parameters (which is a bit of a hack in the first place) with arbitrary IDs that clashed with actual parameter IDs. this really does cause bugs when it happens; several places in norns codebase look up parameters based on their ID, and IIRC there is some use of reverse-lookup structures with ID as hash key and parameter index as data. so you would get things like PSET data containing garbage (or norns LVM actually panicking before exit/shutdown) because it found the separator "parameter" instead of the real one at PSET creation time.

so, we had quite a few of these bugs due to script authors retrofitting their scripts with the new divider params. i put the warning in to help with that.

i don't know that it would be practical / performant to add parameter type checking to every place where you might perform a parameter lookup. maybe some of these checks have already been added... if there has been a lot of churn on parameter since i looked at it, then i encourage other contirbutors to re-evaluate.

but as far as i understand.... i'd really encourage us instead to just prevent ID collisions. maybe we can add helper behavior for creating separators, that will check for collisions, append/prepend a delineating string to the ID, or something. (like name=foo, id='foo' actually gets {name='foo', id='div_foo'}... remember the ID doesn't have to match the display name.)

[xmacex]

Looking at the code, the intent is apparently already there :)

https://github.com/monome/norns/blob/main/lua/core/paramset.lua#L145-L172

maybe it is about a case slipping through e.g. if the separator exists before the parameter or vice versa, I'll try to read when I'm not a train to a performance I'm excited about (Florentina Holzinger, Laurel Halo, Surgeon, Lady Starlight, Amnesia Scanner, Dasha Rush and others)

The warning is a good feature and already saved me from some bugs of my own making, and helped me find a but I made before it was introduced.

A minimal resolution would be, I guess, to be a bit more inclusive in the error message, and guide developers/artists toward checking both parameter and separator IDs.

Maybe it already does, and it's just my reading.

[catfact]

oh i see, looks like @dndrks added some specific stuff for separators etc

[dndrks]

hi hi! hope y'all are both well :)

separators were made more flexibly-scriptable objects with this PR, so they can be programmatically hidden and shown. since they don't have actions associated with them, they're not as precious as other parameter types, but their scripting ID's definitely have some importance.

eg.

function init()
  params:add_separator("scale")
  params:hide('scale') -- i want to hide the separator
  _menu.rebuild_params()
  params:add_option("scale","scale",{'dorian','phrygian'},1) -- this steals the 'scale' ID from the separator
end

function show_separator()
  params:show('scale') -- i want to show the separator
  _menu.rebuild_params()
end

if it helps, i remember the intent behind the messages (and the additional cases I added) being:

• if a non-separator/action-driven param is added with the same ID as any other parameter, the "clobbering" warning prints and the non-separator/action-driven param steals the scripting precedence of the ID. in the example above, show_separator() will never show the separator, because the option-style parameter has taken the ID.
• if a separator param is added with the same ID as any other non-separator parameter, the ! separator ID <scale> collides with a non-separator/action-driven parameter, will not overwrite prints and the non-separator parameter is not overwritten
• if a separator param is added with the same ID as another separator, the ! stealing separator ID <scale> from earlier separator prints and the last-added separator steals scripting precedence

from the conversation above, it sounds like it'd be helpful to print something which fits the scripting model of 'separators are static UI elements' (which was the case for a long time) -- eg. if a non-separator/action-driven parameter steals an ID already-assigned to a separator, we should report that differently from an action-driven parameter stealing from another action-driven parameter.

@xmacex , do you have any preferences/suggestions for the language of the 'your action-driven parameter stole an ID from a separator parameter which is useful to have uniquely identified, but isn't as catastrophic as if you stole the ID from another action-driven parameter' message? :)))))

[dndrks]

small bump -- @xmacex , any additional thoughts strike ya?

[xmacex]

Hiya.

Now that you explain above in https://github.com/monome/norns/issues/1698#issuecomment-1723607806

the intent behind the messages (and the additional cases I added) being

That makes good sense and seems to cover all cases (the intent itself – it's more of an an engineering question whether the case matching logic works as intended) and seems super good!! So good that the message tells clearly tells precicely which ID is the one getting clobbered! Nice work 👨‍🍳👌

Maybe close? This is also quite a niche issue, innit?

Idea to maybe request feedback from initinere maintainer artist about clobbering message clarity

Since this came up in the context of the initenere (best script cover image by the way, I chuckle every time) comment of Shiftr doing exactly as requested by the clobbering warning and reaching out to the maintainer artist and wider community via our lovely forum, I wonder whether the script maintainer artist vicimity would volunteer to serve as an informant about whether the message is useful for them to recognize from the clobbering message alone what the situation is. The message on the forum says

!!! ERROR: parameter ID collision: scale
! please contact the script maintainer - this will cause a load failure in future updates
! BEWARE! clobbering a script or mod param

[xmacex]

Well, it is all here in the docs very clearly from "Separators and groups also have ID’s" onwards.

...why did I even open this issue?