scenerygraphics / sciview

sciview is a tool for visualization and interaction with ND image and mesh data
BSD 2-Clause "Simplified" License
67 stars 17 forks source link

API design choices and leaking objects #239

Closed kephale closed 1 year ago

kephale commented 4 years ago

A related problem, the scenery API is tightly linked to GLVector but we have grown the Vector3 data structures in SciView to complement imagej-mesh and imglib2 interfaces with backings of various data types.

A common use case is:

mySphere = sciView.addSphere(new DoubleVector3(17,17,17), 13)

but this isn't really the solution we want, because there are other use cases that it doesnt satisfy.

For example, if we want to utilize the scenegraph for managing sets of Nodes one currently has to do something like:

Group g = new Group()
for k in range(10):
  Sphere s = new Sphere(1,10)
  s.setPosition(ClearGLVector3.convert(new DoubleVector3(17,17,17)))
  g.addChild(s)

This doesn't feel like the right way to do this, and I'm exaggerating the conversion in there, but practically speaking, Vector3's are strictly better than GLVector when one is doing things within the ImageJ ecosystem.

Now what I don't know is how @ctrueden would approach this to address the leakage of Nodes, especially because it would be kind of silly to mirror all of scenery's Nodes within SciView.

My point is that there are 2 things that need to be directly addressed:

skalarproduktraum commented 4 years ago

So I don't see why leaking scenery objects is necessarily a bad thing, could you elaborate? And I agree that mirroring everything in sciview is not a good solution, and also kind of runs counter to the purpose of the framework.

On GLVector, I have no problem kicking that out and replacing it with either @kephale's Vector3, or JOML's. GLVector being there is kind of a historic artifact, and I have been patching it with functionality I required through time, and I actually want that to be someone else's problem 😆For the sake of type safety, GLVector is also a bit of a pain, because it can have an arbitrary number of components, so Vector2/3/4 is actually waaaay nicer. Switching to Vector3 would be a quite major endeavour though, and would need some time (and testing). @kephale, did you ever do any performance benchmarking with Vector3? Is there an associated matrix class?

In terms of the API, it might be nice to actually provide constructor parameters for parent and position? Or for the moment, at least an overload of setPosition that accepts Vector3?

ctrueden commented 4 years ago

SorryI don't have much time at the moment to scrutize the SciView or Scenery APIs, but I agree with @skalarproduktraum that in principle, leaking Scenery API is not "bad" per se. One reason it would be cool if the SciView class could encapsulate everything such that no method had any graphics.scenery class arguments or return values is: could there ever be a possibility of switching away from Scenery internally? That scenario seems highly unlikely to me in reality. SciView will always be backed by Scenery and only Scenery, and if that ever changed, the SciView API would break backwards compatibility anyway to account for it. No?

I do have concerns about the proliferation of different vector interfaces and classes across Scenery, SciView and imagej-mesh, but again, I don't have time right now to dive in and propose any sort of unifying thing. In principle though it would need to be a common dependency of Scenery and imagej-mesh, right? Which contains the vector data structures? At least the interfaces?

kephale commented 4 years ago

Great, if the leaking issue is not a concern, then all the better. I'm happy to stay attached to scenery objects.

@ctrueden got it. That comment of yours had just been lurking in the back of my mind and I wanted to address it while we're in a productive zone.

@skalarproduktraum FWIW, Vector3 does have JOML backing https://github.com/scenerygraphics/sciview/blob/master/src/main/java/sc/iview/vector/JOMLVector3.java.

The point of using Vector3 in SciView was to avoid what would have been an inevitable replacement of GLVector, and to make an interface that gets all the benefits of imglib2. I haven't done any benchmarking, which would certainly be useful but is more applicable to scenery than SciView. So far SciView design choices have been on the "let's make something that works and integrates as nicely into the ImageJ ecosystem as possible, and if it is efficient all the better" side instead of the graphics choice of "let's favor speed first". I don't want to force that choice onto scenery, and I think that is the wrong direction to focus on.

Precisely because replacing GLVector in scenery would be so time consuming that doing such a replacement is not really even an open suggestion. I am really just trying to figure out how to make sure that user scripts stay thin and don't need to make (from the user's perspective) arbitrary conversions between types just because they are necessary. We can't really punt the decision by saying that scenery should adopt Vector3 because that will just cause a lot of work to come to a standstill.

That said, I think the usage of GLVector needs to be eliminated from all user-facing scripts. If it comes at a performance hit for the time being, so be it.

kephale commented 4 years ago

And just to add, I do think that Vector3 stuff has to be promoted to its own dependency, although the backed implementations may need to live elsewhere (e.g. ClearGLVector3 and JOMLVector3)

kephale commented 1 year ago

@skalarproduktraum @ctrueden I'm happy to close this. I think it is pretty clear that sciview will stay linked to scenery.