problems with sample code

[jrtom]

• ShowLayouts, SubLayoutDemo don’t work (layout creation needs refactoring as with VertexCollapseDemoWithLayouts)
• TwoModelDemo is busted (NPE in BasicEdgeRenderer.paintEdge; GraphicsDecorator is null)
• VertexCollapseDemo is busted (attempts to reuse an edge object already in use)
• GraphEditorDemo
  • parallel edges not rendering as parallel (strangely, edge labels appear to be OK)
  • failed edges (dragged edges whose release point is not on a node) are not being cleared

[tomnelson]

I have been looking into how the refactoring of BasicEdgeRenderer, BasicRenderer, etc broke this code (TwoLayoutDemo). I hope to get a chance to fix it. I'm not sure why some of the changes were made (meaning: someone changed things for a reason, and just changing them back may not be the best solution)

…

On Mon, Aug 7, 2017 at 12:08 AM, jrtom ***@***.***> wrote: - ShowLayouts, SubLayoutDemo don’t work (layout creation needs refactoring as with VertexCollapseDemoWithLayouts) - TwoModelDemo is busted (NPE in BasicEdgeRenderer.paintEdge; GraphicsDecorator is null) - VertexCollapseDemo is busted (attempts to reuse an edge object already in use) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#113>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGbB7oj5Ks0VU4QwNNOzLTA30A6KU2Fks5sVo24gaJpZM4Ou-VW> .

[tomnelson]

I pushed a branch that fixes TwoModelDemo. I will make a PR for it after I have a look at some of the other failed demos to see if the fix is related, otherwise I'll make a different PR for issues related to graph creation.

[jrtom]

While it may have been a bad idea (at least in the sense that it broke something), the reason I made the renderer classes (for example) stateful is that I was trying to make them easier to work with. It's not like a given renderer is likely to be used with multiple render contexts, after all, and it makes the API cleaner if they have state rather than having to supply it for each method call.

The obvious potential risk (which it looks like I fell prey to) is that you fail to update the state when it needs updating, and for that we could consider using listeners.

I'm willing to consider going to back to making the renderers stateless, though.

[tomnelson]

In the original design, I (sort of) followed the java drawing classes in that some context is passed to every draw command. It's more of a functional programming design. We could leave the renderers stateful as long as we don't mind recreating the renderers in every draw loop iteration (bad idea).

I really, really don't like that the RenderContext now holds the Network as part of its state.

I will continue to work on the branch that fixes all the demos. I understand that going back to a functional design may not sit well with everyone. Part of my misgivings about doing this at all is that for a long time I've felt that the need for a java/java2d/swing based rendering module is kind of old fashioned. I suspect that more people would like a javascript implementation these days.

[jrtom]

Following the lead of the Java drawing classes is a reasonable basis for an argument that the renderers should be stateless, definitely. I'm not sure I see how making them stateful means that we have to recreate them in every draw loop iteration (which I agree would be an incredibly bad idea, for sure); can you expand on that a bit?

I don't have access to decent cross-references for JUNG at the moment (no IDE on this machine) so I'd have to see what use (if any) is being made of RenderContext.getNetwork().
The only use that PluggableRendererContext itself makes of the Network appears to be in the constructor to pass it to other objects that one might argue should also not be stateless, i.e., EdgeShape and the edge indexing function; if getNetwork() is never called then we clearly don't need to keep the Network as part of the state of the PluggableRenderContext.

(Side point: it makes me somewhat unhappy that the current visualization system is Network-centric, i.e., it doesn't support Graphs or ValueGraphs. I don't yet have a good solution for this, though, and there may not be a good solution absent changing the public type hierarchy...again.)

I don't necessarily object in principle to returning to a more functional design.

As for the old-fashionedness of the rendering module: to be perfectly honest, I'm not wedded to JUNG continuing to provide a visualization module at all, at least as long as I'm the main person responsible for maintenance (if others including you start to take a more active role again, that would change the calculus). Many people have found it useful to have a visualization system as part of JUNG, and--thanks in large part to you and @danyelf--the one we have is fairly flexible in a number of ways. However, I don't have as good a grasp on the intricacies of the visualization system as I should, it's highly inefficient in some ways (e.g. it lacks proper spatial data structures), and it's definitely an added maintenance burden.

So if there were a good general-purpose graph visualization system that used the same (common.graph) data model as JUNG 3.0 now does, but I'm not aware of one, I'd be perfectly happy to point to it and say "this is a good choice if you want to visualize your graph". At that point I'd support flushing the JUNG visualization capabilities entirely; that would allow me to focus on maintaining and extending the parts of the system that I'm more familiar with.

Hope this clarifies a few things. :)

[jrtom]

So I did a bit of poking around re: RenderContext.getNetwork() and where it's currently used.

The uses can be summarized as follows:
(1) EditingGraphMousePlugin uses it to reject non-mutable graphs. That doesn't require the graph itself, and moreover we could just let that fail later (rather than trying to fail fast).
(2) BoundingRectangle* need the graph in order to be able to calculate the bounding rectangles for each element of the graph. Formerly it got the graph from the Layout, now it gets it from the RenderContext.
(3) The Renderer classes (BasicEdgeRenderer, e.g.) formerly got their graph instance (which they need either for the topology information or for the node/edge sets) from the Layout, now they get it from the RenderContext.

Part of the issue is that the Layout used to be more stateful than it is: that used to be what was responsible in many cases for hanging on to a reference to the graph for purpose of the visualization system. I changed that to make it easier for the layout algorithms to work with Graph and ValueGraph as well as Network objects, but the side effect was that anything that was depending on the Layout to have a handle to the graph had to get its reference to the graph in some other way.

It seems to me that the right object to hang onto the graph instance is ultimately the VisualizationModel, and everything else (layout, renderers, render context, etc.) should get the graph from that. Note, however, that the Layouts are still stateful in the sense that they maintain node positions (and a set of nodes for which they are responsible) so they'd need to be explicitly notified if the VisualizationModel's graph is updated.

[tomnelson]

A lot of the visualization code sprang from thoughts like 'i wonder if we can do this...'.
Now I'm starting to question the use of generics in the rendering code because for the most part, the V and E instances are nothing more than Map keys. It seems silly to demand that we have a VisualizationViewer<String,Number> for example, and then make demos where we change the graph in the VV to something else. I take a lot of blame for this.
We may not even want generics in Layout and its implementations.
It would be nice to be able to easily create a visualization that is agnostic about graph types.
I'm experimenting with various ideas.

[jrtom]

The reason to keep the generics is the main reason why one uses generics in the first place: for type safety.

I mean, the graph types themselves don't care about the node and edge types, because they are (as you say) effectively just keys. But that's the intent. Maps and Collections don't care about their element types, either, but it's still useful for them to have APIs that are defined in terms of those types.

Yes, there are demos where the graph itself is untyped (because in the various collapse demos, the nodes can be either individual objects or collections of objects) but that strikes me as being an edge case (as it were) that we don't want to design around--and we can't, really, anyway.

If the VisualizationModel is to keep a handle to the graph, then it needs to have the same generic types as the graph, I think, or we lose information.

That said, I'm happy to kick ideas around, or to take a look at whatever design you have in mind (although a design doc might be helpful at this point).

[tomnelson]

I still look at this from time to time. Until I see how things end up with regard to Layout and Nework/Graph, I feel disinclined to do a lot of work that may be irrelevant. We have to do something, though. I see that Network is 'final' in DefaultVisualizationModel, yet it is carried as mutable state in many other places. If we intend to support demos that let the user pick different graphs to display, we certainly don't want it to be 'final' anywhere.
Visualizations that allow choosing of a variety of graphs may not even exist outside of our own demo code. They also run into the problem of parameterizing the outer reaches of the visualization viewer, etc for one V,E type and then switching to another. I don't see an obvious best-path now.

[tomnelson]

Are you still interested in flushing the jung-visualization and jung-samples modules?

[jrtom]

Status update:

• SubLayoutDemo seems to have some refresh issues: sometimes when a layout gets changed, some of the edges or nodes don't immediately get repainted until a click on the background or similar triggers a refresh.
• ShowLayouts, TwoModelDemo, GraphEditorDemo, and NodeCollapseDemo seem to be fine.

I'm going to close this issue and open a separate one focused on SubLayoutDemo.