Circle, Rectangle, and Line annotations #98
Conversation
There was a problem hiding this comment.
would be nice to abstract some geometry render logic so there is less duplicative logic between annotations and nodes/edges
|
|
||
| node: Node | ||
| fill: Circle | ||
| strokes?: CircleStrokes |
There was a problem hiding this comment.
conditionally rendering strokes until an object is visible is a perf boost. should consider doing the same for fills
There was a problem hiding this comment.
replaces nodeStrokes
There was a problem hiding this comment.
ended up not using this in favor of Texture.WHITE but kept it around in case there is use for it
src/renderers/webgl/utils.ts
Outdated
There was a problem hiding this comment.
moves to utils/webgl
| @@ -141,11 +137,14 @@ export type RectangleAnnotation = AnnotationBase<'rectangle'> & { | |||
| style: AnnotationStyle | |||
| } | |||
|
|
|||
| export type TextAnnotation = AnnotationBase<'text'> & { | |||
There was a problem hiding this comment.
modeling annotations by geometry and removed TextAnnotation. It is more straight forward to just allow shapes to be filled with text via a content prop if desired.
| } | ||
|
|
||
| for (const annotation of this.annotations) { | ||
| if (lookup[annotation.id] === undefined && this.lookup[annotation.id]) { |
There was a problem hiding this comment.
shouldn't the second this.lookup[annotation.id] condition not be necessary? e.g.
for (const annotation of this.annotations) {
if (lookup[annotation.id] === undefined) {
annotation.delete()
}
}?
| render() { | ||
| const isVisible = this.visible() | ||
|
|
||
| if (isVisible && this.annotation.content && !this.text) { |
There was a problem hiding this comment.
other renderer classes initialize and teardown objects in the update method, and only move/mount/unmount in the renderer. my hunch is that that's a better pattern than initialing Text here, and LineStrokes below. might be missing something, though.
There was a problem hiding this comment.
In large graphs, there is a sizable effort put on the app to instantiate required objects in each object's renderer; in most of these renderers the resulting object is not visible so the load is unnecessary. There are cases where instantiating 5K+ PIXI.Text objects simultaneously actually leads to crashes. We gain a substantial boost in performance if we lazily create these objects until the moment they are first needed.
It makes sense to me to align this logic with the render's animation frame because this is where we apply the object's position (inclusive of any interpolation) as well as addressing culling/visibility. This allows us to reduce the times we are required to call the visible method.
There is potentially an argument for deferring this lazy instantiation to the RenderObject themselves (i.e. Text, LineStrokes, etc.). However, this would still require creating an internal class instance for every object yet this is likely less of a lift than instantiating any PIXI instance.
There was a problem hiding this comment.
could rework visible to optionally accept arguments x, y similar to EdgeRenderer so this logic is moved to update
The issue with moving this lazy creation to update is that this only executes when the object changes, not when the viewport changes. This means sometimes the RenderObject will not be created on the frame it becomes visible.
| const shouldStrokesMount = isVisible && this.renderer.zoom > MIN_STROKE_ZOOM | ||
|
|
||
| if (shouldStrokesMount && !this.strokes && this.annotation.style.stroke) { | ||
| this.strokes = new LineStrokes(this.renderer.annotationsContainer, this.fill, this.annotation.style.stroke).moveTo(this.x, this.y) |
| const isVisible = this.visible() | ||
|
|
||
| if (isVisible && this.annotation.content && !this.text) { | ||
| this.text = new Text( |
There was a problem hiding this comment.
ditto, feels like initializing (and tearing down) the Text object here, and the RectangleStrokes object below, should happen in the update method. and only mounting/unmounting/moving/resizing should happen at render time.
| private hitArea: EdgeHitArea | ||
| private strokes?: LineStrokes |
There was a problem hiding this comment.
Is the new LineStrokes object used to support edges with multiple strokes, so an extension of the edge style spec (which currently only supports a single line)? Cool if so.
There was a problem hiding this comment.
yes exactly. There is probably some fine tuning to be done here, such as wrapping additional strokes around arrows etc.
| const strokesShouldMount = isVisible && this.renderer.zoom > MIN_STROKE_ZOOM | ||
|
|
||
| if (strokesShouldMount && !this.strokes && this.edge.style?.stroke) { | ||
| this.strokes = new LineStrokes(this.renderer.edgesContainer, this.lineSegment, this.edge.style.stroke).moveTo(this.x0, this.y0) |
There was a problem hiding this comment.
ditto, feels like this should be created/torn down in the update method, and only positioned here.
closes #90