Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rounded polygon shapes (e.g. `round-triangle`) #2496

Merged
merged 6 commits into from Sep 6, 2019

Conversation

@josejulio
Copy link
Contributor

commented Aug 9, 2019

Feature request: Add the following rounded node shapes: round-triangle, round-diamond, round-pentagon, round-hexagon, round-heptagon, round-octagon, round-tag

OP follows:

--

I would like to add a new shape, would something like this be accepted into the code base (once finished)?

round-triangle-vs-triangle

@maxkfranz maxkfranz self-requested a review Aug 9, 2019
@josejulio josejulio changed the title Add round-triangle shape WIP: Add round-triangle shape Aug 9, 2019
@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

I'd prefer that if this sort of shape is added that the code be made a bit more general to support N-sided rounded polygons. That way, those shapes could be added in future.

We might want to keep the rectangle case separate, because it may be a bit simpler and cheaper that way.

@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I'd prefer that if this sort of shape is added that the code be made a bit more general to support N-sided rounded polygons. That way, those shapes could be added in future.

We might want to keep the rectangle case separate, because it may be a bit simpler and cheaper that way.

Understood, thanks!

@josejulio josejulio force-pushed the josejulio:round-triangle branch from 0d6e79d to a782c43 Aug 16, 2019
@josejulio josejulio marked this pull request as ready for review Aug 16, 2019
@josejulio josejulio changed the title WIP: Add round-triangle shape Add round-triangle shape Aug 16, 2019
@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I generalized the code to draw a rounded polygon, but I'm reusing the line intersection and point inside tests (polygonIntersectLine and pointInsidePolygon)

I think we can use these for now, while they are not 100% accurate with the rounded shape, they are really close and i don't think they would degrade the experience of the user.

WDYT?

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

I agree in principle, but I'll try it out to confirm.

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

I think it should be more accurate, mostly because of the visual result on the corners:

Screen Shot 2019-08-21 at 3 36 42 PM

Here are approaches for the hit test and the intersection:

round-polygon-hittest-interaection

For the hit test, we check whether the given point is inside the cut-off polygon shape (just the points, without the round corners). We already have those points, and we already have a hit test function for polygons. So the only part that remains for hit tests, if the polygon check fails, is to check whether the point is inside any of the corner circles. That's just a distance squared comparison with the centre point of each circle -- if the square distance is less than the square radius, then the point is inside.

For the intersection, we use a similar approach. First we consider the cut-off polygon. We don't need to consider the corner sides, so we use the existing finite line intersection function for each main side. We also get the intersections with each corner circle. There should be an existing function for intersecting with ellipses. You just take all the intersection points and choose the one with the smallest distance from the given point. Again use square distance comparisons to avoid having to do expensive square root calculations.

@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Thanks for the detailed explanation, I'll work on this.

@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

I'm starting to check this, but i have some questions:

For the intersection, we use a similar approach. First we consider the cut-off polygon. We don't need to consider the corner sides, so we use the existing finite line intersection function for each main side.

I see that there is also a padding, I'm assuming (from what i understood from the other code) that the padding is added to each side of each axis.

You just take all the intersection points and choose the one with the smallest distance from the given point. Again use square distance comparisons to avoid having to do expensive square root calculations.

From what I can see this is already handled here:

let arrowEnd = math.shortenIntersection(

Is that correct?

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

From what I can see this is already handled here:

No, that's to avoid the end of the line going outside of the arrow shape. Without this, for example, the triangle arrow shape would have the line ending on the tip of the triangle.

Scan Aug 29, 2019 at 15 38

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

I see that there is also a padding, I'm assuming (from what i understood from the other code) that the padding is added to each side of each axis.

Yes, but I think that's just historical. Offhand, I'm not sure why the padding was a separate parameter. Currently, the padding value is just passed as 0 to all those functions. The width and height include the padding instead. If you like or if it makes things simpler, you could remove the padding completely from those functions.

@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

No, that's to avoid the end of the line going outside of the arrow shape. Without this, for example, the triangle arrow shape would have the line ending on the tip of the triangle.

Thanks, I was asking because i don't see polygonIntersectLine doing this. It seems to return every intersection with any line:

export const polygonIntersectLine = ( x, y, basePoints, centerX, centerY, width, height, padding ) => {

@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Yes, but I think that's just historical. Offhand, I'm not sure why the padding was a separate parameter. Currently, the padding value is just passed as 0 to all those functions. The width and height include the padding instead. If you like or if it makes things simpler, you could remove the padding completely from those functions.

Ok, I'll remove the padding.

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Thanks, I was asking because i don't see polygonIntersectLine doing this. It seems to return every intersection with any line:

The structure for that function seems to be tailored to nodes. So you consider a finite line from a point outside the node to the centre point of the node. That usually results in only one or zero intersections. If you have a weird polygon (like a concave shape where the centre point is hollow), I suppose there could be conflicts.

In order to resolve that issue and to make that function easier for you to reuse, it probably makes sense to put all the intersections in the intersections array (x1, y1, x2, y2, ...). If the length is greater than two at the bottom of the function, then we need to use the squared distance approach to determine the closest intersection point in the list. Given that it seems there is the assumption that only one point is returned for this function, I think it's safe to return only the closest intersection.

@maxkfranz

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Or you could keep track of the min square distance as you iterate.

josejulio added 5 commits Aug 9, 2019
 - Missing a specific function for line intersection and checkPoint
   Currently using the polygon functions which are not 100% exact
   (the round corners)
 - round-diamond
 - round-pentagon
 - round-hexagon
 - round-heptagon
 - round-octagon
 - round-tag
@josejulio josejulio force-pushed the josejulio:round-triangle branch from a782c43 to f1a1a6b Sep 4, 2019
@josejulio

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@maxkfranz I finished the intersection and hit test for the rounded polygon.

I had to update the points to pre-compute some points to make the rendering and checks easier at the expense of more points (I add one point for every initial point)

I also added some more round shapes, I hope that it's OK.

Here are some GIF's of the demo.
The node's background turns red on mouseover and blue on mouseout.

round-triangle
round-diamond
round-tag

@maxkfranz maxkfranz added this to the 3.11.0 milestone Sep 6, 2019
@maxkfranz maxkfranz changed the title Add round-triangle shape Rounded polygon shapes (e.g. `round-triangle`) Sep 6, 2019
@maxkfranz maxkfranz merged commit 4a3874e into cytoscape:unstable Sep 6, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxkfranz

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Merged in for 3.11.0. Thanks!

@josejulio josejulio deleted the josejulio:round-triangle branch Sep 6, 2019
@maxkfranz

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

Link to the relevant demo: http://js.cytoscape.org/demos/node-types/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.