Border radius

[EliotRagueneau]

Associated issues:

• New edge type: smooth-segments #3201

• border-radius property for node shapes with rounded corners #3198

• This PR adds border-radius support, both for rounded polygons and rounded rectangle

• This PR adds round-segments & round-taxi, as well as their respective radius properties

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.

[EliotRagueneau]

I'm going in hollidays until the 08/01/2024, so I posted this pull request though not yet fullly complete.

TODO

• Store corner data rather than recalculating it for the draw, intersectLine and checkPoint functions in node-shapes.js generateRoundPolygon().
  • I've prepared what to be stored with round.js getRoundCorner(), but I wasn't sure where to store it.
  • The previous way of pre calculating points and unit vectors was at the node shape generator level, therefore prior to any access to radius data. Need to find a place to store. them and reuse them wherever needed
• Add support for padding with border radius as discussed in border-radius property for node shapes with rounded corners #3198
• Generate documenation for the new properties
• Generate demo for the new edge types
• Do further testing to make sure that there are never problems

Of course, feel free to complete some of those tasks while I'm away, and to give me feedback on some aspects.
This is a much bigger PR than all the previous ones I've made, so help would be really appreciated

[maxkfranz]

Pinned

[EliotRagueneau]

Hi @maxkfranz , I hope you had nice holidays.

Could you please give me some advice on where to store pre-calculated results for rounded node shapes ?

Store corner data rather than recalculating it for the draw, intersectLine and checkPoint functions in node-shapes.js generateRoundPolygon().

I've prepared what to be stored with round.js getRoundCorner(), but I wasn't sure where to store it.
The previous way of pre calculating points and unit vectors was at the node shape generator level, therefore prior to any access to radius data. Need to find a place to store. them and reuse them wherever needed

[maxkfranz]

Happy new year.

Caching the values is a bit more complicated than the decision of where to store the points. They need to be invalidated in the same cycle as the other cached values.

I'll get back to you with details in the next day or so.

For edges, caching is probably worthwhile, but for nodes it probably doesn't make much difference.

[EliotRagueneau]

Reminder:

• Check if the new roundness types are well supporting underlay
• Check if the new roundness types are well supporting overlay

[maxkfranz]

Some initial notes:

1. Cached points can be stored in ele._private.rstyle.
2. The caching of the points is initiated here: https://github.com/cytoscape/cytoscape.js/blob/unstable/src/extensions/renderer/base/coord-ele-math/edge-control-points.js#L928-L932
3. allpts is also used for calculations and it may need to be updated, especially for a large radius on an edge.

[EliotRagueneau]

For the edges, I've managed to cache what is need I believe.
allpts has been edited as well to handle middle edge arrows. I've even included a new midVector to simplify the drawing of the arrows afterwards, for their orientation

[EliotRagueneau]

As you can see bellow, I've just tweaked a bit the outline formula to adjust the roundness of the outline, so that it looks neat.

Also I've checked for overlay and underlay, and both are working great for the new edge curvatures

[maxkfranz]

Just a point to consider re. naming before we merge this in:

[maxkfranz]

Looks reasonable. There are some minor linter errors to resolve.

Are there any other tests or points of consideration before merging this in?

[EliotRagueneau]

The thing that worries me the most really is the rounded node shapes which are now less optimised than before because we are not caching the unit vectors, I was hoping to be able to cache the rounded corners themselves within the nodes.

You said it was okay, but I'm a bit suprised because we are calculating them in 3 different functions for each round shapes, and potentially, if the user moves a node, we need to recalculate the roundness again and again to calculate the arrow head position using the intersection function. But if you say that's alright, I'm fine with it.

[maxkfranz]

Of course, it’s better to cache.However, the nodes have texture caching applied at a level above the points caching.  So, the points only need to be recalculated for drawing when the texture cache misses.One area that would be affected is the hit tests, for determining when you click on a node, for instance.It’s up to you if you want to apply caching of the points now.  I think it would be better with caching, but it could also be added later.  Only people who use the new, custom border radius would be affected, right?  If so, it’s suboptimal but not a regression.On Jan 12, 2024, at 05:22, Eliot Ragueneau ***@***.***> wrote: The thing that worries me the most really is the rounded node shapes which are now less optimised than before because we are not caching the unit vectors, I was hoping to be able to cache the rounded corners themselves within the nodes. You said it was okay, but I'm a bit suprised because we are calculating them in 3 different functions for each round shapes, and potentially, if the user moves a node, we need to recalculate the roundness again and again to calculate the arrow head position using the intersection function. But if you say that's alright, I'm fine with it. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[EliotRagueneau]

Well no, with the current code, the new way to calculate corners is applied to all the rounded polygons, not just those for which the user specified the border radius. I did this to avoid having yet other shapes like "customisable-round-rectangle", and because the current implementation of the rounded shapes is failing when the width and the height are dissimilar.
Therefore the lack of node caching would is harmful anytime a user use a "round-xxx" shape

[EliotRagueneau]

I've implemented the node caching, it's a bit more dirty than the previous caching since it is at the node level instead of the abstract shape definition, but the cache is well being used whenever we can, and updated whenever we need.
I think this covers all the concerns I had.

[maxkfranz]

Looks great. Merged.

If you'd like to add anything else to this feature, or you notice any issues, just let me know.

[EliotRagueneau]

I noticed while testing it that for round-segments, it tends to create loops if a point is collinear to the previous and the following point, e.g. if A, B and C are on a single line, then B will have a loop on top of it.

In theory, users shouldn't put unnecessary control points, and these are unnecessary since they do not make the edge move in any way, but it can be confusing. Do you think I should prevent this issue from happening? Like by ignoring collinear points? Is it too late to fix it for next release?

[maxkfranz]

There are all sorts of cases where people can create invalid beziers.

We print some warnings in the console for the worst cases that get detected with no additional computational cost (e.g. the bezier can't be drawn at all).

If you think there can be an inexpensive check to detect this case, then we can either automatically correct the points or we can print a warning in the console. Someone might create collinear points when creating control points interactively. Consider the case where you have a few points and it's easier to just drag one point to be collinear rather than to delete it. That example demonstrates that autocorrection would generally be preferable.

Is it too late to fix it for next release?

We don't need to be too strict about the release date of 3.29.0. Let's consider 2024-02-12. Would that be enough time to add your proposed fix?

[EliotRagueneau]

yes that's probably more than enough, thanks a lot!
Will fix that ASAP :D

[maxkfranz]

Look forward to it

[EliotRagueneau]

It seems my new commits are not showing in this PR, so I'll create a new PR