SciView.addNode, Node.addChild, Node.(set)Parent ambiguity

[moreApi]

@xulman and I had a chat during lunch and he mentioned his struggels with adding childs to parent nodes in sciView.

In SciView/Java there is a setParent method generated which looks like it is on the same level as addChild. And worst of it all, both are, used alone, wrong. In SciView one has to use SciView.addNode (which directly adds the node under scene, so no other parent) or addChild + SciView.publishNode to add a node in away that it shows up in the inspector and is known to imageJs services.

In scenery and kotlin this is a bit more clear. There is the addChild method and the parent property. This hints that a child should be added by using the function.

possible solutions on the scenery side

• making node.parent internal, so there is no setParent generated
  • add a node.detachFromParent or node.removeChild to get the option back to do parent = null for deleting nodes.
• extend parent::set to mirror node.addChild (could be hard to avoid a loop)

possible solutions on the sciView side

• use scene.onChildrenAdded scene.onChildrenRemoved to listen to scene graph changes and call sciView.publishNode there. Remove or simplyfy sciView.addChild then.
• somehow document / teach all users that they should not use node.parent and node.addChild alone

[kephale]

I think SciView.addChild could be removed. This is API simplification, so it is great, and I don't see any usages.

I like the idea of listening to scene graph changes and calling publishNode automatically.

[kephale]

Since this deals with API, I'd like to resolve it for 1.0.0. I agree this is an important point.

[moreApi]

@skalarproduktraum wants to do this task but he needs time to mull about it

[kephale]

Ok, this is related: #504

[xulman]

sciview (not scenery) user's perspective:
(based on my fresh experience of a newcomer ('cause I have realized I have forgotten most of the sciview coding sadly))

• o = SciView.add*() methods I understood them as factories that give me Node-derived obj that I decide where to hook in the scene tree.. but they got immediately hooked to the root

• my expectation was that I must always do a call to insert the newly created o to the scene -- I anyway want to adjust the created obj later.. not always it is comfortable to create a bulk of code that would feed the addSphere() fully in one go

• later I realized I can do o = new Sphere(...) but then how do I get into the scene?

  • by assigning it a parent is a way, perhaps not necessarily the most obvious... does assigning a parent (aka building a structure) also mean adding it to the scene? argument to get rid of the setParent() method
  • by addChild(o) that would work for me better... still the question of doing structure vs. inserting remains, no?

• any later o.setParent() had no effect (AFAIR)

• calling parent.setChild(o) I think created a duplicity.. the node o was not moved but was added (and was listed twice in the insepctor panel)

[xulman]

my take would be a bit anti-Kotlin chatty:

exemplified on Spheres:

• createSphere(..)
• insertNodeIntoScene()
• insertNodeUnderParent(parentNode)

shortcuts:

• createAndInsertSphere(...) -- inserts into scene root
• createAndInsertSphere(..., parentNode)

Notice the policy: explicit creation and explicit adding -- often I prefer to first prepare the object before I introduce it into the scene

but that's breaking API... let me come back with a better proposal in a while

[kephale]

I found myself doing this in a script yesterday:

sciview/src/main/resources/sc/iview/scripts/find_and_count_cells_cardona.py

Lines 89 to 93 in 7fcf71f

	node = Sphere(radius, 20)
	node.spatial().setPosition(Vector3f(peak).mul(scale))
	node.material().setDiffuse(Vector3f(1, 0, 0))
	ch1.addChild(node)
	sciview.publishNode(node)

[kephale]

Just brainstorming:

One major point for me though: convenience functions that do everything in one call are useful when you're in SciJava scripting land, where types (let along generics) becomes frustrating (e.g. in dynamic languages).

Convenience create-and-add functions

• node = SciView.addSphere() as well as other shapes.
• This creates and adds a Node to the root of the scene.
• I believe these should stay, but maybe could be convinced otherwise if equivalently useful functions could be shown to work in all relevant use cases (e.g. at least both building a tool that leverages sciview like @xulman, and SciJava scripting).

More custom usage

TBH, I think the example I linked above is pretty reasonable. If you want to do something custom in the scene graph, then do it with scenery and use sciview to publish the node.

In theory if there was a real need to add child nodes and publish them in sciview simultaneously, then we could add some extension functions to Node.

[That said, this needs to be clearly presented and documented, because this hasn't been a usage pattern that we have been promoting yet.]

[edit again:]

This leads me to suggest that we should clean up API on the sciview side, and add more examples/documentation of both cases.

[xulman]

my problem was that I, until you sent the post a while ago, failed to have a nodes hanged under some another node of mine (which was in the root of the scene) and have them visible in the inspector... I tried several combinations of setting parent, adding child,... and was getting either duplicitly visibly items or none.... I was missing the publishNode() trick

I agree that the example is good and I like it actually (I can get solo basic obj, incrementally set it up, and only then decide to make it available in the scene)... just failed to get the whole construction using the current API

[kephale]

@xulman it is 100% sciview's fault not yours.

[xulman]

no one's to blame (except myself)

[kephale]

Let's just blame the software 👼

[moreApi]

'publishNode()' is 3 days old. There was no way to do this properly before^^

[xulman]

I like the pattern, I understand the API must stay (not break), and I'm okay with that (if I may say).

Just thinking of adding one/two convenience functions to make it crystal clear for newcomers:

• remove the (from users' perspective non-functional) setParent()
• add parentNode.addChildAndShowIt(child) { this.addChild(child); getSciView().publishNode(child); }
• and perhaps the very same thing just from the child's perspective: child.publishUnderParent(parent) { ...}

the latter is a helper to easier find the "publishing code", the block would then look more node-ish:

node = new Sphere()
node.spatial()....foo...
node.material().... blah...\
node.publishUnderParent( someParent );

(@kephale me back from the lunch... sorry for the delays)

[kephale]

'publishNode()' is 3 days old. There was no way to do this properly before^^

You know an API change is good when you already assume it has been around forever :D

[kephale]

The new methods that @xulman suggests sound great to me and would be pretty straightforward as extension functions

[xulman]

can I do PR?

[kephale]

That would be wonderful!

[kephale]

Maybe let @skalarproduktraum know you're diving into it.

[xulman]

I was about to code this and file the PR and while thinking about it, it is again hitting the wall between scenery and sciview...

the proposal of removing setParent() -- this is purely a scenery thing

adding child.publishUnderParent( someParent ); or parentNode.addChildAndShowIt(child) { this.addChild(child); getSciView().publishNode(child); } is wanting to extend the functionality of Node (in scenery) and then reach out to publishing the node (which I understood is mainly sciview thing)

so, I'm thinking of creating just one SciView method that takes both childNode and parentNode as params...
but discovered (and they are actually noted in the original/very first post above):

• SciView.addChild( node ) - adds node into the scene (gets rendered) but not listed in the inspector panel
• SciView.addNode( node ) - as above plus it publishes by default and can hook even under a user-given parent if one provides some block: N.() -> Unit = {} (3rd param to addNode())

I don't understand this 3rd param and failed to fake it anyhow (like providing a function that does nothing)...

nevertheless, feels like addNode() has everything needed:

• introducing a node to the scene (get's rendered)
• adding to the inspector by default, but can be postponed for performance reasons when a burst of nodes is added, postponed to the last node (which, as a side effect, will also show all previously not-shown nodes)
• add to the scene/root and also to any node

(this is the SciView.addNode() after the block param was removed:

this adds two spheres, later being a child of the former, and both are displayed in the inspector panel, yay!

[xulman]

SciView.addChild(n) we should maybe leave (for API compatibility)... it's just adding nodes to the root, and not displaying them, but we have now the explicit re-read trigger SciView.publishNode(n).. perhaps we should add
SciView.addChild( n, underThisParent ) to have it complete

so user would either be using the mighty addNode() or will be walking smaller explicit steps via addChild() and eventually publishNode()

[xulman]

equivalent to the addNode() as explained just above:

the code lives here... would need to do something about the now-testing addNodeB()

[kephale]

I am open to deprecating SciView.addChild.

[kephale]

I only really use SciView.addNode (except for the convenience functions like addSphere etc.).

I share @xulman's frustration with the block function argument. It is really hard to use that properly from Java, and probably even worse from SciJava scripting languages.

[xulman]

it's not so bad at all once the programmer recalls/understands everything...

---

just to explain:
I'm clearly not the smartest programmer, which is good here :-) (-:, I can easily play the role of an everyday user (the programer user), and I'm reporting around what one might come accross (confusion can easily turn into roadblock and loss of a sciview user)

[kephale]

Closed by #521

I expect any related ideas would be captured by #489