Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ossia-max] Avoid creating intermediate nodes #653

Open
evanmtp opened this issue Dec 3, 2020 · 10 comments
Open

[ossia-max] Avoid creating intermediate nodes #653

evanmtp opened this issue Dec 3, 2020 · 10 comments

Comments

@evanmtp
Copy link
Contributor

evanmtp commented Dec 3, 2020

When an ossia.parameter with a multi-level address is created, ossia creates intermediate nodes for each level of the hierarchy. For example, if an ossia.parameter is created with the address audio/gain/interpolation, the following nodes are created:

device:/model/audio
device:/model/audio/gain
device:/model/audio/gain/interpolation

If another ossia.parameter is then created with the same name as one of these intermediate parameters (e.g. /out/final), it will wind up with a numerical suffix:

device:/model/audio
device:/model/audio/gain
device:/model/audio/gain.1
device:/model/audio/gain/interpolation

This makes it easy to accidentally create duplicate node names, which can cause some confusion when e.g. retrieving values or storing presets.

Please see these demo patches:

intermediateNodes.zip

My question is: is this auto-creation of intermediate nodes desired or useful behaviour? I don't understand what the purpose of this is, but I may very well be missing something. If it's not necessary for some reason, could this behaviour be prevented?

Possibly related to #628 and #559 . Tested with b206e46.

@evanmtp
Copy link
Contributor Author

evanmtp commented Dec 3, 2020

I also tested with an older build that I've been working with (7c49e83). In this case, the test patch results in the following nodes being created:

device:/model/audio
device:/model/audio/gain
device:/model/audio/gain/interpolation

In this case, the node with .1 appended is not being created. I still wonder, though, is it necessary for these intermediate nodes to be created at all?

Intuitively, what I would expect from this patch is just two nodes corresponding to the ossia.parameters that have been explicitly created:

device:/model/audio/gain
device:/model/audio/gain/interpolation

@avilleret
Copy link
Contributor

short answer : it is not possible to avoid creating intermediate nodes, by design.

long answer :
libossia is based on node tree. node could have parameter. and each node has a parent node but the root of the tree (this is how we define the root : it has no parent).
If you want to create an ossia.parameter with address /foo/bar without creating the foo node, then what will be the parent node of your parameter ?

concerning the auto-incrementation of nodes names, this is a feature that allows you to easily duplicate model.
for example you can load n instances of a model with address /foo/bar.1 in a poly~ and then each instance will have its instance number as a suffix in its address.
This is pretty handy.

in the case of your example, its a bit more convoluted. it's an edge case that has not been specified iirc.
You first create [ossia.parameter audio/gain/interpolation] which creates all the needed intermediate nodes.
The you create [ossia.parameter audio/gain]. It will try to create the node audio/gain which already exists, then it creates a new node gain.1 under the audio node.

What is not specified is what we should do when we want a parameter foo while a node foo already exists but without a parameter .
Currently it create a child node called foo with a parameter. The resulting namespace is : /foo/foo.
I thought this is more straightforward like this but maybe not. see:

----------begin_max5_patcher----------
374.3ocuT1saBCBEG+51mBBW2YJsUctWkEyBVYNLs.AnttX7cevg141pSaL5
t4jb9h+7iyocebDdkrkYvnmPOihh1GGEAg7Ah57iv0z1xJpAJCWyLF5FFNIj
yxZsPbA0kQQK+JihZKeiK17hlUZCJjULIMAQx814DuMKaRJZYWGhlZtnhYAg
xNFT1X6iR5hxWChJWs8g48BFJy9ghETCiQK8YNDG6MIiDPA6c2wNfOklKrWj
sEodad14YibR1RGxVA91AfzX3zKBPwT.f4WC.mX3j++Lb.1lnnZ2RnkoQuJk
mgTRN.4zYv1HY50vZ1PVy9aVStK7VKWypFGqDf0E42nwJYjiUPFbEW76+w.2
Te7extQ1nK6OptOtQGurqYFKWPsbo3a0jGp4jutiUm7QnSw.cB3QUpcLsoqX
PB2PbqT6ceLAb4hfK7Vh0rc795CEP0tAm0M0Zzv0B2Nq.GZ0Mj0hFNzcrGNm
jvBxw+35avuGEeH9SbgFakG
-----------end_max5_patcher-----------

If we decide that in the case above, the ossia.parameter create only a parameter attached to the already existing node, then your case will be solved, because since audio/gain  already but doesn't have a parameter, [ossia.parameter audio/gain] will create a parameter attached to that node.
But this should be discussed as it will break some patchers and presets.

@evanmtp
Copy link
Contributor Author

evanmtp commented Dec 3, 2020

My preference in cases like would be that if a node exists that doesn't have a parameter attached, and then a parameter is created with the same address, the parameter should be attached to that node. Having parameters get auto-incremented in cases where there's a node with no parameter seems quite counterintuitive to me.

In building models, I often create parameters that partly share the same address, but then terminate at different depths. For example,

acc/pitch
acc/pitch/invert
acc/pitch/active
acc/pitch/offset

This winds up creating an extra auto-incremented parameter:

acc/pitch.1

@navid and @petervanhaaften have used the same pattern in their modules, and I suspect that this is something that other users would intuitively do as well. The current behaviour thus seems to me like it might cause some confusion.

@avilleret
Copy link
Contributor

what do @maybites @jln- @matcham @vincentgoudard and maybe @bltzr and @jcelerier think about that ?

@maybites
Copy link
Collaborator

maybites commented Dec 3, 2020

@avilleret I don't understand your foo/foo example in your long answer and how this relates to this problem. it looks like expected behavior to me. I always thought the [ossia.model] is a parent node of the [ossia.parameter] inside the same patcher.

thats because I tend to take @evanmtp side of the argument and I don't see how this bites each other.

@vincentgoudard
Copy link
Contributor

vincentgoudard commented Dec 4, 2020

AFAIC, with @evanmtp example, I get this as a namespace, which matches what I would expect (using v1.1.0a4 980a883) :

print: namespace ossia_max_device:/myModel
print: namespace ossia_max_device:/myModel/audio
print: namespace ossia_max_device:/myModel/audio/gain
print: namespace ossia_max_device:/myModel/audio/gain/interpolation

As for @avilleret example, this is also what I would expect, as I also consider a "model" (or device) to be the "parent" of any patcher attached to it. In this respect, it make sense that a parameter foo attached to a model foowould create a parameter address foo/foo.

But as @maybites says, I don't really see how those two bite each other either.

@bltzr
Copy link
Member

bltzr commented Dec 8, 2020

I might have missed some points, but in the zipped examples there shouldn't be any duplicated node (in my current version, 1.0.4-7c75235, there isn't any)

@evanmtp
Copy link
Contributor Author

evanmtp commented Dec 16, 2020

Just made a build from the latest commit (#776351d8b) and tested - still getting duplicate nodes:

Screen Shot 2020-12-16 at 11 55 15 AM

@avilleret, @bltzr , what do you think at this point? Seems like @maybites, @vincentgoudard and I are in agreement, and @petervanhaaften and @navid are as well.

@avilleret avilleret added the bug label Dec 22, 2020
@avilleret
Copy link
Contributor

I think this is fixed : I don't see any extra parameters with 2da51f3 (should be fixed in v1.2.0-rc1 too)

@vincentgoudard
Copy link
Contributor

I still have the duplicate node issue with v1.2.0 built 3476fe7 .

print: namespace ossia_max_device:/myModel
print: namespace ossia_max_device:/myModel/audio
print: namespace ossia_max_device:/myModel/audio/gain.1
print: namespace ossia_max_device:/myModel/audio/gain
print: namespace ossia_max_device:/myModel/audio/gain/interpolation

@vincentgoudard vincentgoudard reopened this Jan 7, 2021
avilleret added a commit that referenced this issue Jan 12, 2021
avilleret added a commit that referenced this issue Jan 12, 2021
@jcelerier jcelerier reopened this Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants