Node outlines #3158
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -74,6 +74,10 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| let borderColor = node.pstyle('border-color').value; | ||
| let borderStyle = node.pstyle('border-style').value; | ||
| let borderOpacity = node.pstyle('border-opacity').value * eleOpacity; | ||
| let outlineWidth = node.pstyle('outline-width').pfValue; | ||
| let outlineColor = node.pstyle('outline-color').value; | ||
| let outlineStyle = node.pstyle('outline-style').value; | ||
| let outlineOpacity = node.pstyle('outline-opacity').value * eleOpacity; | ||
|
|
||
| context.lineJoin = 'miter'; // so borders are square with the node shape | ||
|
|
||
| @@ -85,6 +89,10 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| r.colorStrokeStyle( context, borderColor[0], borderColor[1], borderColor[2], bdrOpy ); | ||
| }; | ||
|
|
||
| let setupOutlineColor = ( otlnOpy = outlineOpacity ) => { | ||
| r.colorStrokeStyle( context, outlineColor[0], outlineColor[1], outlineColor[2], otlnOpy ); | ||
| }; | ||
|
|
||
| // | ||
| // setup shape | ||
|
|
||
| @@ -114,7 +122,7 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| } | ||
| } | ||
|
|
||
| let drawPath = () => { | ||
| if( !pathCacheHit ){ | ||
|
|
||
| let npos = pos; | ||
| @@ -127,12 +135,16 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| } | ||
|
|
||
| r.nodeShapes[ r.getNodeShape( node ) ].draw( | ||
| ( path || context ), | ||
| npos.x, | ||
| npos.y, | ||
| nodeWidth, | ||
| nodeHeight ); | ||
| } | ||
| } | ||
|
|
||
| let drawShape = () => { | ||
| drawPath(); | ||
|
|
||
| if( usePaths ){ | ||
| context.fill( path ); | ||
| @@ -246,7 +258,57 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| if( context.setLineDash ){ // for very outofdate browsers | ||
| context.setLineDash( [ ] ); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let drawOutline = () => { | ||
| if( outlineWidth > 0 ){ | ||
| // because outline and border are drawn along the same path, | ||
| // draw outline at border width plus twice the outline width | ||
| context.lineWidth = borderWidth + outlineWidth * 2; | ||
| context.lineCap = 'butt'; | ||
|
|
||
| if( context.setLineDash ){ // for very outofdate browsers | ||
| switch( outlineStyle ){ | ||
| case 'dotted': | ||
| context.setLineDash( [ 1, 1 ] ); | ||
| break; | ||
|
|
||
| case 'dashed': | ||
| context.setLineDash( [ 4, 2 ] ); | ||
| break; | ||
|
|
||
| case 'solid': | ||
| context.setLineDash( [ ] ); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| drawPath(); | ||
|
|
||
| if( usePaths ){ | ||
| context.stroke( path ); | ||
| } else { | ||
| context.stroke(); | ||
| } | ||
|
|
||
| const gco = context.globalCompositeOperation; | ||
| context.globalCompositeOperation = 'destination-out'; | ||
|
There was a problem hiding this comment. Do we need to worry about what happens when an outline renders over another already rendered element here? Wouldn't it clip the outline above that too? There was a problem hiding this comment. I was thinking about that too the other day. The GCO is fine for things like edge arrows, since they're so small. However, there are important use cases for having semitransparent nodes -- e.g. with overlap -- and the GCO would visually delete things. It would be best to use a clip operation here. It can be a bit cheaper if you use the There was a problem hiding this comment. I've managed to update the rendering to clip instead of use gco, but only by rendering a rect with the node shape cut out, and clipping to that. I might very well be missing something, but clipping to the node shape has the opposite effect of clipping the outer portion of the outline instead of the inside portion. There are still a bunch of irregular shapes whose outline exceeds the node width/height + outline width though, and I'm not sure how to accurately expand the rect bounds in those cases, and when calculating the overall node bounds in There was a problem hiding this comment. That's a good point. Here are some initial thoughts: (1) Two multipliers, one for width and one for height, might suffice for predefined shapes. One possibility is something like (2) Assuming we go with (1), how would we address custom (3) The proper way to calculate the adjusted bounding box would be to take the intersection of the relevant lines in the shape, when they're pushed out by the outline width. This would be more expensive, and it would mean that we may as well use a path for the outline, since we'd have the points to form the path anyway. There was a problem hiding this comment. Taking a step back, pretty much all of the complexity that's arisen has come with trying to solve the use case of the background and/or border having a partial opacity, and not wanting any of the outline below to show through. But we already have that with borders, where a border with partial opacity effectively creates two visual borders due to the fact that the inner half overlaps the node shape: 
In an ideal world, it might be worth considering rendering both borders and outlines such that they never overlapped other elements. In practice though this has proven to be less straightforward than it would seem, whether accomplished by offsetting the stroke or by clipping, and there's a chance that the computational cost would end up outweighing the aesthetics, making it hard to justify the tradeoff. While adding outlines admittedly moves us closer to that potential decision point, I wonder if we could/should consider simplifying the outline implementation for now by not worrying about clipping the inner portion, and consider that broader rethink around how strokes are rendered (whether border or outline) down the road. There was a problem hiding this comment. What if we rebrand the feature? What you're describing sounds like a The implementation would be identical to the existing borders, apart from the draw order. There was a problem hiding this comment. @maxkfranz that could be a possibility as a fallback, but my preference would be to keep the outline framing if possible. I will give the suggestion you proposed above (calculating the path by pushing out the points by the outline width) a shot. |
||
|
|
||
| r.eleFillStyle( context, node, 1 ); | ||
|
|
||
| if( usePaths ){ | ||
| context.fill( path ); | ||
| } else { | ||
| context.fill(); | ||
| } | ||
|
|
||
| context.globalCompositeOperation = gco; | ||
|
|
||
| // reset in case we changed the border style | ||
| if( context.setLineDash ){ // for very outofdate browsers | ||
| context.setLineDash( [ ] ); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| @@ -276,6 +338,8 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
|
|
||
| context.translate( gx, gy ); | ||
|
|
||
| setupOutlineColor(); | ||
| drawOutline(); | ||
| setupShapeColor( ghostOpacity * bgOpacity ); | ||
| drawShape(); | ||
| drawImages( effGhostOpacity, true ); | ||
| @@ -296,6 +360,8 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s | ||
| context.translate( pos.x, pos.y ); | ||
| } | ||
|
|
||
| setupOutlineColor(); | ||
| drawOutline(); | ||
| setupShapeColor(); | ||
| drawShape(); | ||
| drawImages(eleOpacity, true); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge deal but this function should probably be named differently, especially since the core render methods are prefixed with the
drawprefix. Maybe something likeensurePathwould be more accurate semantically?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good