Node outlines #3158

npedrini · 2023-08-22T23:03:29Z

Associated issues: #3156

Adds node outlines as described in #3156. Pushed this one up a bit early, but will be monitoring the associated issue and integrate any feedback that comes in.

Reiterating the feature as laid out in the issue description, node outlines support the same properties as borders, but with the addition of an outline-offset prop, and are drawn outside the border/node shape.

Much of the implementation mirrors how borders are rendered, but because outlines are drawn outside the border/node, the node shape needs to be redrawn at a slightly larger size than the core, filled shape. For that reason, I opted to move the path caching to a getPath method that both the shape rendering and drawOutline function can use, rather than duplicating that caching logic.

10px, dashed outline:

10px double outline w/ 4px offset:

Triangular node w/ 10px outline:

Rhomboid w/ 10px outline, 8px offset:

TODO:

Check offset rendering for other node shapes (noticed offsets seem off for some of the tests above)
Update node bound calculation to account for outline if present (right now larger outlines get clipped if they exceed bounds)

Checklist

Author:

The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
The associated GitHub issues are included (above).
Notes have been included (above).

Reviewers:

All automated checks are passing (green check next to latest commit).
At least one reviewer has signed off on the pull request.
For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

Closes #3156

maxkfranz · 2023-08-23T16:25:09Z

Is the rhomboid example intended to be offset or flush with the node shape? How do more complex shapes, like star, fare?

npedrini · 2023-08-24T16:09:08Z

Is the rhomboid example intended to be offset or flush with the node shape? How do more complex shapes, like star, fare?

The rhomboid example was with an 8px offset (edited the caption to that effect) but it should definitely have a consistent offset on all sides, as all outlines should. To your question, rhomboid turns out to be just one of a handful of irregular shapes that aren't being outlined consistently, and where doing so looks to be less straightforward than expected.

I'm going to work on revisiting the rendering logic with some of those cases in mind. I'll be away next week but hope to follow-up soon with an update.

maxkfranz · 2023-08-30T18:19:39Z

I see. Other shapes to watch out for may include star and polygon.

One way to simplify the implementation would be to drop support for doubled outlines and outline-offset. The implementation would just be a stroke and a clip, and it would always work for all shapes. Obviously, that has trade-offs in the richness of the feature.

Looking forward to your updates.

npedrini · 2023-09-05T06:09:28Z

Thanks @maxkfranz. I've managed to get some better results by massaging the node width/height/offset for some of the irregular shapes:

(4px outline w/ 2px offset)

Not sold on the approach though as it feels somewhat brittle, and I can't say there's much rhyme or reason to the values I've settled on. Also, rhomboids are still problematic. Here's what some of that looks like though:

let multiplier = outlineWidth / 2 + outlineOffset * 2 + borderWidth;

if (shape === "triangle" || shape === "round-triangle") {
  multiplier = outlineWidth + outlineOffset + borderWidth;
  npos.y = -multiplier * .3;
  width += multiplier * .6;
  height += multiplier * .6;
} else if (shape === "vee") {
  npos.y = -multiplier / 2 * .3;
  width += multiplier / 2 * 3;
  height += multiplier / 2 * 3;
} else if (shape === "tag" || shape === "round-tag") {
  npos.x = multiplier * .1;
  width += multiplier * .2;
} else if (shape === "star") {
  npos.y = -multiplier * .15;
  width += multiplier * .9;
  height += multiplier * .9;
} else if (shape === "diamond" || shape === "round-diamond") {
  multiplier = outlineWidth/2 + outlineOffset * 2 + borderWidth/2;
  width += multiplier * .66;
  height += multiplier * .66;
} else if (shape === "pentagon" || shape === "round-pentagon") {
  npos.y = -multiplier * .1;
  width += multiplier * .2;
  height += multiplier * .2;
} else if (shape == "heptagon" || shape == "round-heptagon") {
  npos.y = -multiplier * .03;
  width += multiplier * .03;
  height += multiplier * .03;
}

Noting that I've seen some unexpected rendering anomalies when using larger line-widths, but that seems to occur with borders as well.

The inner edge of outlines for rounded shapes don't line up with the corresponding border segment as can be seen above, but I'm not sure that's something we can do much about (?):

One way to simplify the implementation would be to drop support for doubled outlines and outline-offset. The implementation would just be a stroke and a clip, and it would always work for all shapes. Obviously, that has trade-offs in the richness of the feature.

I'm open to exploring this path. Can you go into a bit more detail about what Canvas operations you see this approach using, and/or posting some basic pseudo-code?

maxkfranz · 2023-09-06T12:46:59Z

I'm open to exploring this path. Can you go into a bit more detail about what Canvas operations you see this approach using, and/or posting some basic pseudo-code?

For solid outlines: Reuse the border code. Double the border width. The border is centred on the shape, so half is inside and half is outside. Clip the doubled border you’ve drawn with the node shape to remove the inside portion.

npedrini · 2023-09-12T07:32:57Z

Thanks @maxkfranz. Went ahead and dropped support for the double stroke style and the outline-offset property for simplicity's sake, and to get this one over the finish line.

4px solid outline

1px solid outline

Ended up using globalCompositeOperation to knock out the inner border portion rather than clip. Also, because the outline is drawn before the node shape and drawOutline also expects the node shape to have been drawn, I moved the bit that draws the path if uncached to a drawPath function that both drawShape and drawOutline can call.

Let me know if anything stands out.

maxkfranz · 2023-09-13T00:21:42Z

globalCompositeOperation

That can essentially be a cheaper method of clipping. Great.

Let me know if anything stands out.

It looks solid.

I'll review in more depth later this week. Let's get this merged.

npedrini · 2023-09-14T09:52:00Z

src/extensions/renderer/canvas/drawing-nodes.js

@@ -114,7 +122,7 @@ CRp.drawNode = function( context, node, shiftToOriginWithBb, drawLabel = true, s
    }
  }

-  let drawShape = () => {
+  let drawPath = () => {


Not a huge deal but this function should probably be named differently, especially since the core render methods are prefixed with the draw prefix. Maybe something like ensurePath would be more accurate semantically?

Sounds good

danprince · 2023-09-14T13:58:29Z

src/extensions/renderer/canvas/drawing-nodes.js

+      }
+
+      const gco = context.globalCompositeOperation;
+      context.globalCompositeOperation = 'destination-out';


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?

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 nodePathCache for the clipping rather than having to redraw/regenerate the node body.

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 bounds.js if an outline is present. I can apply an arbitrary multiplier, but that feels like a hack which would likely have adverse implications on node layout, etc.

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 bb.w = k_w * outlineWidth + otherMeasuresForWidth.

(2) Assuming we go with (1), how would we address custom polygon shapes? This leads into (3). Alternatively, we could lean on the existing bounds-expansion property. The dev, in that case, must specify their own expansion of the bounds to accommodate the outline on their custom polygon.

(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.

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.

What if we rebrand the feature?

What you're describing sounds like a under-border rather than an outline. That way, you can still use it as an outline -- with the opacity caveats etc. -- but it would have clear expectations for other devs who use the feature. Nobody would be disappointed about the transparent node use cases, since it's obvious that those use cases aren't suited to an under-border.

The implementation would be identical to the existing borders, apart from the draw order.

@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.

Sounds good

stale · 2023-10-17T13:14:30Z

This issue has been automatically marked as stale, because it has not had activity within the past 14 days. It will be closed if no further activity occurs within the next 7 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

maxkfranz · 2023-10-18T13:56:21Z

Pinned

npedrini · 2023-10-18T14:44:56Z

Still not perfect, but was able to get relatively consistent results by using the expandPolygon helper to push out the points for drawing the outline path for complex shapes, and scaling the simpler ones. With this approach I was also able to bring back the outline-offset property and support for doubled outlines:

4 px outline-width, 4px outline-offset

The round- shapes are obvious outliers though, and need some more work to render consistent with the others. It seems like the same bounds expansion approach should work there, but it almost looks like the resulting points are out of order (?) and not quite sure how to order them properly post-expansion.

Here's what the round-hexagon looks like, for example:

For bounds calculation, we don't have access to the points, so I'm simply applying a multiplier that accounts for the outline. Curious if there's a better way, and if not, perhaps we could consider asking devs to rely on the bounds-expansion property as a fallback.

maxkfranz · 2023-10-18T18:29:35Z

For bounds calculation, we don't have access to the points, so I'm simply applying a multiplier that accounts for the outline. Curious if there's a better way, and if not, perhaps we could consider asking devs to rely on the bounds-expansion property as a fallback.

As long as it works in practice, it’s fine. It may be necessary to use different multipliers for different shapes, and different multipliers for x versus y. We don’t want the bounds to be too far off from reality.

The round- shapes are obvious outliers though, and need some more work to render consistent with the others. It seems like the same bounds expansion approach should work there, but it almost looks like the resulting points are out of order (?) and not quite sure how to order them properly post-expansion.

Instead of aiming for perfection here, how about we just use the implementation for the non-round shapes for the outlines of the corresponding rounded shapes?

Most users probably wouldn’t notice the difference for many outline use cases, anyway. It could always be improved in future, if necessary.

How does that sound?

npedrini · 2023-10-19T07:31:41Z

I updated the round shape outline rendering logic to apply a scale multiplier and slight offset where necessary to get them more in line with the other shapes, and tweaked the bounds calculation to use a standard formula with a few exceptions where needed. Feel pretty good about it now all considering, so @maxkfranz, please take a good look when you get a chance and let me know if anything stands out.

maxkfranz · 2023-10-23T21:54:01Z

Great. I'll test it out shortly. Thanks, @npedrini

maxkfranz · 2023-10-26T14:37:17Z

@npedrini, I tested out the PR and overall it looks great.

I put some changes to the debug page for testing in this branch: pr/3158/test https://github.com/cytoscape/cytoscape.js/tree/pr/3158/test

When you run npm run watch, it opens up the debug page and live-rebuilds the lib for it so you see your changes right away. In the pr/3158/test branch, you can see that I've put up the code from the node shapes demo for testing:

There are a number of helpful tools in the right sidebar. For this feature, the 'Selected BB' (bounding box) buttons are useful. Just select a node and then click one of the buttons to see its bounding box.

One thing that could be improved in this PR is to tighten up the bounding box adjustments a bit. What does your formula look like for the multipliers? For some shapes especially, the bounding box seems a bit loose. See the video below:

Screen.Recording.2023-10-26.at.10.36.21.mov

npedrini · 2023-10-27T16:03:52Z

Thanks @maxkfranz, the bounds tool in the debug mode was indeed useful in getting a visual indication of what the bounds were looking like. I've tightened them up a bit and they're looking more consistent to me:

Screen.Recording.2023-10-27.at.4.47.40.PM.mov

For most shapes I'm calculating an x- and y-scale with outline and offset relative to the node bounds, then expanding the bounds by the pixel equivalent of the delta in either direction. For some of the more irregular shapes I'm adjusting those scales with arbitrary multipliers that seem to achieve the desired results. I've been testing that with small outline settings like you've added in that debug branch, as well as exaggerated values.

Do we need to update the outline-offset property definition to trigger bounds?

{ name: 'outline-offset', type: t.size, triggersBounds: diff.any }

maxkfranz · 2023-10-27T16:07:07Z

Looking good.

Re. bounds triggering, yes. Anything that could alter the bounds when the prop is changed needs to have a diff set. For continuous values, generally 'any' is appropriate -- as you suggested.

maxkfranz · 2023-11-09T17:32:01Z

@npedrini @danprince, let me know if there's anything else you'd like to consider in this PR.

Otherwise, let's merge this in.

danprince · 2023-11-09T18:37:10Z

@maxkfranz Don't think so, looking great on our end!

maxkfranz · 2023-11-10T13:28:59Z

Good stuff.

Merged

npedrini added 5 commits August 22, 2023 12:52

generalize path caching into getPath method

dfdf1b7

add outline style props

52862fb

update node rendering to draw outlines

39be812

update node cache key to include outline styles

38f58f5

update docs

3218d25

npedrini added 3 commits September 11, 2023 10:12

remove outline-offset property

ac3b23c

remove support for double outline style

ded390e

simplify outline rendering

f301088

npedrini commented Sep 14, 2023

View reviewed changes

danprince reviewed Sep 14, 2023

View reviewed changes

stale bot added the stale label Oct 17, 2023

maxkfranz added pinned A long-lived issue, such as a discussion and removed stale labels Oct 18, 2023

draw outlines by expanding polygons

ed2c095

tweak round shape outlines, bounds calc

e66b1fe

tighten up bounds calculations

b245cc7

update outline-offset prop to trigger bounds

d1c0103

maxkfranz added this to the 3.28.0 milestone Nov 9, 2023

maxkfranz merged commit f0c24be into cytoscape:unstable Nov 10, 2023
1 check passed

danprince deleted the node-outlines branch November 10, 2023 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node outlines #3158

Node outlines #3158

npedrini commented Aug 22, 2023 •

edited

Loading

maxkfranz commented Aug 23, 2023

npedrini commented Aug 24, 2023

maxkfranz commented Aug 30, 2023

npedrini commented Sep 5, 2023

maxkfranz commented Sep 6, 2023

npedrini commented Sep 12, 2023

maxkfranz commented Sep 13, 2023

npedrini Sep 14, 2023

maxkfranz Sep 26, 2023

danprince Sep 14, 2023

maxkfranz Sep 14, 2023

npedrini Sep 18, 2023

maxkfranz Sep 26, 2023

npedrini Sep 27, 2023

maxkfranz Sep 28, 2023

npedrini Oct 3, 2023

maxkfranz Oct 3, 2023

stale bot commented Oct 17, 2023

maxkfranz commented Oct 18, 2023

npedrini commented Oct 18, 2023

maxkfranz commented Oct 18, 2023

npedrini commented Oct 19, 2023

maxkfranz commented Oct 23, 2023

maxkfranz commented Oct 26, 2023

npedrini commented Oct 27, 2023

maxkfranz commented Oct 27, 2023

maxkfranz commented Nov 9, 2023

danprince commented Nov 9, 2023

maxkfranz commented Nov 10, 2023

Node outlines #3158

Node outlines #3158

Conversation

npedrini commented Aug 22, 2023 • edited Loading

maxkfranz commented Aug 23, 2023

npedrini commented Aug 24, 2023

maxkfranz commented Aug 30, 2023

npedrini commented Sep 5, 2023

maxkfranz commented Sep 6, 2023

npedrini commented Sep 12, 2023

maxkfranz commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Oct 17, 2023

maxkfranz commented Oct 18, 2023

npedrini commented Oct 18, 2023

maxkfranz commented Oct 18, 2023

npedrini commented Oct 19, 2023

maxkfranz commented Oct 23, 2023

maxkfranz commented Oct 26, 2023

npedrini commented Oct 27, 2023

maxkfranz commented Oct 27, 2023

maxkfranz commented Nov 9, 2023

danprince commented Nov 9, 2023

maxkfranz commented Nov 10, 2023

npedrini commented Aug 22, 2023 •

edited

Loading