Closed moreApi closed 1 year ago
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.
Since this deals with API, I'd like to resolve it for 1.0.0
. I agree this is an important point.
@skalarproduktraum wants to do this task but he needs time to mull about it
Ok, this is related: https://github.com/scenerygraphics/sciview/issues/504
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?
setParent()
methodaddChild(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)
my take would be a bit anti-Kotlin chatty:
exemplified on Spheres:
createSphere(..)
insertNodeIntoScene()
insertNodeUnderParent(parentNode)
shortcuts:
createAndInsertSphere(...)
-- inserts into scene rootcreateAndInsertSphere(..., 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
I found myself doing this in a script yesterday: https://github.com/scenerygraphics/sciview/blob/7fcf71f58ff7771b339cf19fc229bb500c59346d/src/main/resources/sc/iview/scripts/find_and_count_cells_cardona.py#L89-L93
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).
node = SciView.addSphere()
as well as other shapes. Node
to the root of the scene. 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.
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
@xulman it is 100% sciview's fault not yours.
no one's to blame (except myself)
Let's just blame the software 👼
'publishNode()' is 3 days old. There was no way to do this properly before^^
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:
setParent()
parentNode.addChildAndShowIt(child) { this.addChild(child); getSciView().publishNode(child); }
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)
'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
The new methods that @xulman suggests sound great to me and would be pretty straightforward as extension functions
can I do PR?
That would be wonderful!
Maybe let @skalarproduktraum know you're diving into it.
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 panelSciView.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:
(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!
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()
equivalent to the addNode()
as explained just above:
the code lives here... would need to do something about the now-testing addNodeB()
I am open to deprecating SciView.addChild
.
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.
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)
Closed by https://github.com/scenerygraphics/sciview/pull/521
I expect any related ideas would be captured by https://github.com/scenerygraphics/sciview/issues/489
@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
possible solutions on the sciView side