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

Graph layers #605

Merged
merged 38 commits into from May 1, 2017
Merged

Graph layers #605

merged 38 commits into from May 1, 2017

Conversation

ericsoco
Copy link
Contributor

This PR adds a new sample layer, a non-geospatial layer for layout and rendering of a force-directed network graph. graph-layer is a composite layer that delegates rendering to a ScatterplotLayer, LineLayer, and (optionally) IconLayer; graph-layout-layer is an adaptor layer that links a layout (defaults to graph-simulation, which uses d3-force) to the renderer graph-layer.

In this design, the graph layout and rendering are decoupled, to allow flexibility with each.

There is an additional layer of processing in adaptor/ -- these files process data from different formats for use by the specified layout. It might make more sense to call that folder parser/, not sure. These parsers/adaptors allow the layout and rendering of three different sample datasets, which can be toggled via the DATASET const in app.js.

This PR has not gone very far into documentation, and does not add the sample layer to the layer browser; I'll do that work in a subsequent PR once we land on an implementation here that we're comfortable with.

Some notes:

  1. I'm driving the force layout manually, forceUpdate()ing the root component and subsequently tick()ing the simulation via requestAnimationFrame(). Alternately, we could let the layout run independently of rendering, but that feels more magical and hard for devs to understand/manipulate. Also, I'm concerned that running the layout independent of the rendering / React updating might cause stutter in the animation.

  2. Because of 1. above, I have to force deck.gl to update on every simulation tick, even when no props passed into graph-layer have changed. The simulation alpha remains constant while dragging as it's continually being re-warmed, so alpha can't be used as an updateTrigger. Therefore, I'm creating a new object every update while the simulation is still running and using that as an updateTrigger. Is there a less hacky, more explicit way to have deck.gl update on-demand?

  3. graph-layout-layer is passing up the type of picked element via its own getPickingInfo() implementation. Within, I'm determining if a link or node is picked by digging into info.layer.context.lastPickedInfo.layerId. This feels like the wrong way to determine which layer was picked from among graph-layer's sublayers. Is there a better way to get this information from the picking info object? (I can also tell by the structure of the picked datum/object if necessary, but thought I should be able to find the picked layer lowest in the layer stack from the picking info, and so I went digging...)

@ericsoco ericsoco requested a review from gnavvy April 27, 2017 19:04
@coveralls
Copy link

coveralls commented Apr 27, 2017

Coverage Status

Coverage remained the same at 37.359% when pulling 796097b on graph-layers into 2cbe6f7 on master.

@howtimeflies0
Copy link

I need some time to review this change.

Also, we recently created a repo for data files (https://github.com/uber-common/deck.gl-data) so those 100k line data files don't need to be in deck.gl. Let's discuss the correct way to utilize it

@@ -0,0 +1,35 @@
<!-- INJECT:"GraphLayerDemo" -->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since everything else is in the /examples folder (a good initial approach I think), this doc would be better as a README.md in the layer folder in the example at this point rather than part of the official layer documentation.

Copy link
Contributor

@gnavvy gnavvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do it file by file.

import {json as requestJSON, csv as requestCSV} from 'd3-request';

import DeckGLOverlay from './deckgl-overlay';
import {default as GraphBasic} from './graph-layer/adaptor/graph-basic';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we add an index.js to the adaptor directory that handles default exports, so it's less verbose in the app.js?
also, would be helpful to have an inline comment, like in your PR description, briefly mentioning what an adaptor does, in cases users (like me) jump directly into reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while i find that pattern helpful for e.g. library code, in which the contents of the folder fronted by the index.js might grow over time, i think in example code it's less helpful -- there's no reason to assume we'll be adding more adaptors as time goes on, and it's one more step the dev following the example has to process to understand where the imports are coming from. (also, it's a bit magical for those who don't understand that import resolution looks for an index.js first.)

agreed on the comment, will add.

requestJSON('./data/node-icon-atlas.json', (error, response) => {
if (!error) {
this.setState({
iconMapping: response
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no default value for iconMapping in state.

const {lastDragged} = this.state;
if (lastDragged) {
this.setState({
lastDragged: null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no default lastDragged in state.

const GraphAdaptor = dataConfig[DATASET].adaptor;
const graph = new GraphAdaptor(response);
this.setState({
graph,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems graph is set but never used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it twice, immediately after the declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you mean graph is never pulled off of this.state. Good catch.

if (el.source) {
// link
element = (<line
x1={el.source.x + viewport.width / 2}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of adding the offset to x and y for each element, we can keep move it out by adding a
<g transform={`translate(${viewport.width / 2} ${viewport.height / 2})`}>{...}</g>
that wraps the content in the tag in line 295.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.

//
// rendering
//
_renderInteractionLayer(viewport, hovered, clicked) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems the interactionLayer is an SVG layer used for highlighting the selected & related elements.
we might need some descriptions above for explanation.

also, (not in this PR), while I agree that using SVG for highlighting should be performant enough given we are only highlighting a limited number of connected/related elements for the current graph use case, we loose the flexibility to interleave them (Interaction layer will also on top) for 3D graphs. let's discuss this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this whole SVG interaction layer thing is less than ideal. let's discuss later if there's a way to handle this more seamlessly within deck.gl.

hovered: hovered && hovered.object,
clicked: clicked && clicked.object
};
const relatedElements = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this demonstration correct?

// (node) ---------(edge)------------------ (node)
//   ^ related       ^ hovered/clicked       ^ related
//
// (node) ----(edge)---- (node) ------------(edge)---- (node)
//   ^ related  ^ related  ^ hovered/clicked  ^ related  ^ related

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is how highlights are applied, yes. However, the relatedObjects field only contains the nodes at either end of a selected link, or the links attached to a selected node.

I added the relatedObjects field as a convenience for consumers of graph-layout-layer. My implementation of choosing elements to highlight, however, is split across graph-layout-layer::getPickingInfo() (which assigns relatedObjects as I just described), and app.js::_renderInteractionLayer(), which goes one step further to pick nodes attached to links that appear as relatedObjects.

As I explain this, I'm thinking perhaps I should move all that logic into getPickingInfo(). Do you agree that it's a common use case to desire access to nodes connected to a selected node?

this.state.nodes = nodes;
this.state.links = data ? data.links : undefined;
// create new object for updateTriggers diff
this.state.layoutUpdating = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnavvy and I discussed; instead of using an Object, a timestamp is probably more legible to indicate that this is a value changing every update.


const {width, height} = viewport;
const {layoutProps} = this.props;
const {layoutAccessors, linkAccessors, nodeAccessors, nodeIconAccessors} = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm fine with either but other layers use get* as the naming convention for accessors. keep the same for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think get* is good for an individual accessor, but these are maps of accessors, not functions. "Get" is a verb and so implies a function, so it doesn't quite map right for these maps. If you insisted I'd change them but since you're fine with either, I'll leave as-is :)

@howtimeflies0
Copy link

howtimeflies0 commented May 1, 2017

I'm driving the force layout manually, forceUpdate()ing the root component and subsequently tick()ing the simulation via requestAnimationFrame(). Alternately, we could let the layout run independently of rendering, but that feels more magical and hard for devs to understand/manipulate. Also, I'm concerned that running the layout independent of the rendering / React updating might cause stutter in the animation.

Using forceUpdate() for this simple example is okay, but if we were to make this GraphLayer a workhorse for our applications, we might not want the force calculation to run at 60fps.

That being said, one more important question to be answered here is whether we actually want to have GraphLayer (which contains graph data) as a sublayer of GraphLayoutLayer (which contains layout algorithm). It might be worth considering making layout algorithm pluggable to a graph layer instead of the other way around, as user may want to frequently swap layout algorithms for a same set of data.

More over, in the future, we might want to implement layout calculations outside of browser (e. g. in middleware), if we'd like to visualize millions of nodes or combine layout with other algorithms like edge bundling.

@howtimeflies0
Copy link

graph-layout-layer is passing up the type of picked element via its own getPickingInfo() implementation. Within, I'm determining if a link or node is picked by digging into info.layer.context.lastPickedInfo.layerId. This feels like the wrong way to determine which layer was picked from among graph-layer's sublayers. Is there a better way to get this information from the picking info object? (I can also tell by the structure of the picked datum/object if necessary, but thought I should be able to find the picked layer lowest in the layer stack from the picking info, and so I went digging...)

You are right. Determining whether a link or node has been picked shouldn't involve digging into layer.context. This depends too much on internal implementations of deck.gl's picking and will sure be broken if the implementation is changed. It's better that the type of picked entity be encoded somehow to the pickInfo.object, just like what GeoJson layer is doing.

ericsoco added 18 commits May 1, 2017 15:09
Does not run yet, but does lint and compile.
Still WIP: nothing rendering yet.
Rebuild OrthographicViewport on every render.
Correctly format data passed into logic component with timestamp.
Refactor "edges" to "links" to be more d3-idiomatic.
Add `global` package for `window` references.
Import from deck.gl (resolved by webpack on npm run start-local) instead of local filesystem.
Remove unused layer attributes (because of orthographic viewport).
Some other cleanup.
…ph-simulation), and composite layer that connects the two (graph-layout-layer).

This architecture enables the composite layer to swap out both the graph layout and the renderer as necessary.
Note that this refactoring decouples modules across build pipelines, with the rendering layer in the deck.gl package and the composite layer in /examples; this requires a special webpack config to skip transpilation on the composite layer so that it can extend the untranspiled base Layer class.
…so it's more accurate to call it the "layout" layer.
Clean up picking info returned from graph-layer.
ericsoco added 20 commits May 1, 2017 15:09
Use uber color palette
Implement icon rendering example
Bundle layout accessors similarly
Straighten out default props
Not yet connected to graph layout.
Required changing updateTriggers from layout alpha (which stays constant while dragging) to a flag/marker object that is recreated every frame the layout is still updating.
Still not working perfectly due to drag event handling issues in deck.gl.
Tie React updating to animation loop to prevent unnecessary renders and synchronize layout ticks with rendering.
Remove unused state
Add state defaults
Translate container element instead of individual elements
…ucture of event object.

Consolidate logic for "related objects" into getPickingInfo() instead of requiring application to determine nodes related to picked node.
@ericsoco ericsoco merged commit c5ec63f into master May 1, 2017
@ericsoco ericsoco deleted the graph-layers branch May 1, 2017 22:17
Firenze11 pushed a commit to Firenze11/deck.gl that referenced this pull request May 31, 2017
* Initial pass at graph-layer.
Does not run yet, but does lint and compile.

* Iron out compile-time errors.
Still WIP: nothing rendering yet.

* Get graph to render as deck.gl layer.
Rebuild OrthographicViewport on every render.
Correctly format data passed into logic component with timestamp.
Refactor "edges" to "links" to be more d3-idiomatic.
Add `global` package for `window` references.
Import from deck.gl (resolved by webpack on npm run start-local) instead of local filesystem.
Remove unused layer attributes (because of orthographic viewport).
Some other cleanup.

* Refactor to decouple renderer (graph-layer), graph layout engine (graph-simulation), and composite layer that connects the two (graph-layout-layer).
This architecture enables the composite layer to swap out both the graph layout and the renderer as necessary.
Note that this refactoring decouples modules across build pipelines, with the rendering layer in the deck.gl package and the composite layer in /examples; this requires a special webpack config to skip transpilation on the composite layer so that it can extend the untranspiled base Layer class.

* Add graph manipulation UI.

* Starting to implement mouse interaction; committing WIP to bring layer finalization fix on master into branch.

* Create `graph-simple` example without graph data management.

* Add support for (and basic implementation of) accessors and handlers.

* Graph data management is now handled outside of graph wrapper layer, so it's more accurate to call it the "layout" layer.

* Remove d3 dep from root and move into examples

* Implement and pass accessors for display and graph simulation.

* Rename first pass at graph example to graph-old.

* Rename current graph example from "graph-simple" to just "graph".

* Add example adaptors for multiple datasets / formats

* Set up three example datasets and adaptors.

* Add SVG interaction layer.
Clean up picking info returned from graph-layer.

* Implement link picking.

* Highlight connected nodes on interaction.

* Prevent key collisions between hovered and clicked elements

* Display node name (if it exists)
Use uber color palette

* Add icon texture atlas and attributions

* Add grouped icon accessors
Implement icon rendering example

* Clean up node icon accessor compliation in app.js
Bundle layout accessors similarly

* Limit number of nodes in SNAP dataset for perf

* Bundle accessors where useful & possible
Straighten out default props

* Be resilient against missing prop

* Remove unused props

* Remove examples/graph-old

* Wire up drag handlers to deck.gl.
Not yet connected to graph layout.

* Implement node dragging.
Required changing updateTriggers from layout alpha (which stays constant while dragging) to a flag/marker object that is recreated every frame the layout is still updating.
Still not working perfectly due to drag event handling issues in deck.gl.

* Move graph-layer out of src/layers/core, and repackage everything within examples/graph/graph-layer.

* Clear lastDragged state immediately after it's passed down to layout.
Tie React updating to animation loop to prevent unnecessary renders and synchronize layout ticks with rendering.

* Get dragging and clicked to work together correctly.

* Move documentation entirely into layer README.
Flesh out documentation.

* Reduce size of facebook-SNAP dataset and update parser accordingly

* Add comments
Remove unused state
Add state defaults
Translate container element instead of individual elements

* Use datestamp instead of object for forcing updates

* Determine object type by inspecting data shape instead of private structure of event object.
Consolidate logic for "related objects" into getPickingInfo() instead of requiring application to determine nodes related to picked node.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants