Add overlay-shape property

[zakjan]

Minimal possible implementation for #2379

While default overlay shape remains round-rectangle, this gives users the possibility to use another shape.

The result is here:

It's clear it's not perfect for certain shapes, but at least it improves the visual look for the basic shapes.

[maxkfranz]

While it's an interesting idea and it's been tried before, I'm not sure things have changed much since then. If we don't provide a good experience for all the shapes, it's not really worth it. The benefits are marginal, and users are fine with the rounded rectangle. It's familiar -- like the active indicator on links in mobile Safari.

Do you think you could amend your PR to support all the shapes well, with code that would be simple and maintainable?

[demtec]

I would also appreciate this feature. Actually, we are currently doing a manual hack where we are changing the shape in every cytoscape.js instance before we include it in our projects. It's really annoying not to be able to change the overlay shape. I think the devs should be able to choose a different shape when they don't want the default rounded rectangle. Since it will be a configurable thing I don't see an issue with including it into the library.
From the provided image I see that the experience is like 95% perfect, which is more than enough, in my opinion, and the pros are much bigger than the cons here.

[michalhabala]

I've requested the feature in the past because I'm using the overlay to highlight the selected state and since all of the nodes are always circle (in our case) it would be much better to have the possibility to use a circle overlay shape. Frankly, I don't understand why this option is not there already. Otherwise, the library is really great, appreciate all the hard work.

[ondrej2]

This would be really good to have.

[jabyclak]

+1

[zakjan]

support all the shapes well, with code that would be simple and maintainable?

@maxkfranz I'd love to. There is a possibility for an improvement by calculating centroid position of the base shape or custom polygon, and offset the overlay shape to match the centroid. Do you think it would work well? It might not be enough for shapes with sharp angles such as vee.

For custom polygons, does it make sense to add separate overlay-shape-polygon-points? If all shapes are supported well, I think it might be better to restrict overlay-shape values to round-rectangle (default) or inherit (use node base shape) only.

[zakjan]

Changes:

• offset overlay by overlayShape center, improves triangle rendering
• add auto value to follow node shape
• custom polygon is also supported

[demtec]

This looks fantastic! @maxkfranz can you please take a look at it? Seems to be ready to merge.

[maxkfranz]

This looks like a great improvement over the initial result for the examples you've shown. However, there are two errors in your example:

Proposal:

1. Confirm that the changes do not result in drawing beyond the bounding box for the shapes. You can test this in the debug page, where a cyan box can be shown for the selected element. If they do overflow, then the bounding box calculations will need to be amended.
2. Confirm that the approach creates an equidistant padding when the node aspect ratio isn't 1:1. E.g. try 1:3 and 3:1 on the demo page.
3. The overlay-shape should always default to round-rectangle to correspond with built-in browser active-state indicators.
4. All built-in shapes must be well supported.
   1. Alternative (4.i): Use the previous approach for vee. As a compromise since it's user-specified rather than built-in, remove polygon from overlayShape. Remove auto, or auto defaults to roundrectangle for polygon.
   2. Alternative (4.ii): Rather than simply drawing another shape with modified width and height, calculate the outward projections of the list of points in order to make the padding between the shape and the overlay equidistant. This may be necessary instead of (4.1) based on the results of (2).

[zakjan]

Thanks for the detailed feedback! I wondered about the last two concave polygons.

1. Overlay padding is already included with includeOverlays
2. It doesn't. For those cases users could be advised to keep the default shape
3. The default value is round-rectangle
4. Since built-in polygons are generated internally using the same function as custom polygons, custom polygons need to be well supported too. If 2. is a blocker, it seems that 4b. is the way to go.

[zakjan]

The requirement on visually equidistant padding means that overlay shape needs to render rounded corners for each original point, right?

[maxkfranz]

It sounds like you're talking about a line join, right? It looks like the line join should match that of the reference shape. E.g. the joins should probably be rounded for rounded shapes and sharp for sharp shapes.

[zakjan]

I explored the suggested approaches. It seems that the requirement of equidistant absolute padding for all shapes is not feasible.

Even if drawing-shapes.js is rewritten with a new padding parameter in all methods, it would fulfill the requirement for line shapes (drawn with lineTo) only. Curved shapes (drawn with arcTo, quadraticCurveTo, ellipse) would still render non-equidistant padding along the curve.

Is there any other possibility to allow finishing this PR? Limitations of custom overlay-shape can be documented.

[maxkfranz]

I explored the suggested approaches. It seems that the requirement of equidistant absolute padding for all shapes is not feasible.

Even if drawing-shapes.js is rewritten with a new padding parameter in all methods, it would fulfill the requirement for line shapes (drawn with lineTo) only. Curved shapes (drawn with arcTo, quadraticCurveTo, ellipse) would still render non-equidistant padding along the curve.

Could you clarify what you mean precisely with some figures? Clearly, the corners are not going to be equidistant to the node shape given some types of line joins -- even for aesthetically pleasing cases. E.g. the rectangle shape isn't equidistant at the corner, but a user would say that it "looks right".

Basically, if the result would look janky to a user, then a better approach should be used. Let's not get hung up on strict mathematical definitions. The aesthetics are what's important for styling.

In order to officially support this feature, all the built-in shapes must give an aesthetically pleasing result when used with the overlays. Your initially proposed approach was tried in the past, and unfortunately, it caused more problems than benefits. Those who used the feature complained about it (i.e. bad UX and DX). Those who didn't use the feature could obviously live without it.

It would be great to have this feature in the core library. In doing that, we have a responsibility to the greater community to create a positive experience -- for features to work as expected and to minimise incompatibilities wherever possible.

It's definitely possible to have aesthetically pleasing overlay shapes. Granted, the math and implementation may be more involved than your initial implementation. Or you may need a simpler, creative solution. I understand that you're doing this as a part of consulting work for Graphlytic and you probably want to get things resolved quickly for your client, but we need to balance this PR with what's best for the Cytoscape community as a whole.

Thanks for your PR, and I'm looking forward to seeing your next steps. If you need help or want more feedback, you're welcome to comment here. Or if you think it would be faster to clarify and discuss things in a call, we could schedule a quick one.

[zakjan]

Could you clarify what you mean precisely with some figures? Clearly, the corners are not going to be equidistant to the node shape given some types of line joins

Corners are clear, these could be handled by configurable line joins, e.g. so that rectangle overlay can look as rectangle (mitter), or round-rectangle (round), or cut-rectangle (bevel).

I meant equidistant padding along curves. Ideally, padding should enlarge the shape in the normal direction from the curve path. Increasing radius by padding for ellipse, arcTo methods, or moving control point for quadraticCurveTo method, would enlarge the shape in the direction from the curve center instead, so it's mathematically incorrect for non-square aspect ratios. If this is an acceptable drawback, then adding padding parameter to all methods in drawing-shapes.js could be a possible way to go. The question is whether it is worth the effort.

---

I understand your concerns. I wonder what simpler solution is possible though, given the constraints.

I explored an idea of enlarging the overlay by ratio in width/height multiples depending on aspect ratio, instead of current static padding in px, but it didn't have good results for all shapes.

How about implementing overlay-shape with a few basic shapes only (round-rectangle, rectangle, ellipse)? No auto or shapes that are incompatible with current overlay implementation. It would fit my client's usecase, and apparently there might be a demand for it from other users as well.

[zakjan]

I removed complex shapes from the PR, kept round-rectangle, rectangle, ellipse only. It's a pretty minimal change now. Generic support for all shapes can be implemented separately.

[stale]

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.

[demtec]

@maxkfranz could you please take a look at this, if you still see some potential issues? The implementation was reduced to only 3 overlay shapes: "round-rectangle" (current), "rectangle", and "ellipse". Adding the "ellipse" shape is the absolute minimum that we need in our case. Thanks

[maxkfranz]

I’ll try to set aside some time within the next few days. Is there a particular motivation for including the square-cornered ‘rectangle’ option?

…

On Sep 6, 2021, at 1:30 AM, demtec ***@***.***> wrote: @maxkfranz <https://github.com/maxkfranz> could you please take a look at this, if you still see some potential issues? The implementation was reduced to only 3 overlay shapes: "round-rectangle" (current), "rectangle", and "ellipse". Adding the "ellipse" shape is the absolute minimum that we need in our case. Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2881 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHRO43543GPC33OJP56PFTUARGX7ANCNFSM46LWE3KQ>.

[demtec]

Thanks. There's actually no particular motivation for the square-cornered case. We have added it just because it's working well, the proportions are fine with all different sizes of the node.

[maxkfranz]

If that’s the case, might it be a bit better from the point of view of a library user if it were just the two — round-rectangle and ellipse? The ordinary square-cornered rectangle existing in tandem with the round version may give the expectation to users that other shape variants should also exist. I don’t foresee many cases where someone would want the non-rounded version, so it may be best to omit it until there’s demand for it

…

On Sep 14, 2021, at 3:42 AM, demtec ***@***.***> wrote: Thanks. There's actually no particular motivation for the square-cornered case. We have added it just because it's working well, the proportions are fine with all different sizes of the node. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2881 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHRO43JXN3CEATGSUKBGGLUB34GDANCNFSM46LWE3KQ>.

[zakjan]

Thanks for confirming, removed rectangle.

[maxkfranz]

Merged. Will release after #2897 is merged in

[zakjan]

Awesome we got it finished! I'm going to follow up with #2882