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

Taxi edge turn radius (i.e. smooth curve on each turn) #3142

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions documentation/md/style.md
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,10 @@ When a taxi edge would be impossible to draw along the regular turning plan ---
* Note that bundling may not work with an explicit direction (`upward`, `downward`, `leftward`, or `rightward`) in tandem with a turn distance specified in percent units.
* **`taxi-turn-min-distance`** : The minimum distance along the primary axis that is maintained between the nodes and the turns.
* This value only takes on absolute values (e.g. `5px`).
* This property makes the taxi edge be re-routed when the turns would be otherwise too close to the source or target. As such, it also helps to avoid turns overlapping edge endpoint arrows.
* This property makes the taxi edge be re-routed when the turns would be otherwise too close to the source or target. As such, it also helps to avoid turns overlapping edge endpoint arrows
* **`taxi-radius`** : The taxi-radius property applies a curve to a taxi edge, providing a rounded appearance to any 90 degree bend.
* This value should be a floating point number (e.g. `3.14`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This value should be a floating point number (e.g. `3.14`).
* This value should be a floating point length (e.g. `3.14px`).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a clarification


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we cap the radius (see the comments later on), it may be useful to have another sub-bullet-point that notes that the radius should not be too large. It depends on how your other taxi properties are specified, the layout, etc.

* **`edge-distances`** : With value `intersection` (default), the distances (`taxi-turn` and `taxi-turn-min-distance`) are considered from the outside of the source's bounds to the outside of the target's bounds. With value `node-position`, the distances are considered from the source position to the target position. The `node-position` option makes calculating edge points easier --- but it should be used carefully because you can create invalid points that `intersection` would have automatically corrected.


Expand Down Expand Up @@ -717,4 +720,4 @@ Selection box:
Texture during viewport gestures:

* **`outside-texture-bg-color`** : The colour of the area outside the viewport texture when `initOptions.textureOnViewport === true`.
* **`outside-texture-bg-opacity`** : The opacity of the area outside the viewport texture.
* **`outside-texture-bg-opacity`** : The opacity of the area outside the viewport texture.
38 changes: 31 additions & 7 deletions src/extensions/renderer/base/coord-ele-math/coords.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,37 @@ BRp.findNearestElements = function( x, y, interactiveElementsOnly, isTouch ){
var pts = rs.allpts;

for( var i = 0; i + 3 < pts.length; i += 2 ){
if(
(math.inLineVicinity( x, y, pts[ i ], pts[ i + 1], pts[ i + 2], pts[ i + 3], width2 ))
&&
widthSq > ( sqDist = math.sqdistToFiniteLine( x, y, pts[ i ], pts[ i + 1], pts[ i + 2], pts[ i + 3] ) )
){
addEle( edge, sqDist );
return true;
if( rs.edgeType === 'segments' && edge.pstyle( 'taxi-radius' ).value > 0 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that this is intended to be a performant simplification, but it doesn't seem to work as intended, e.g. you get clear false positives on the debug page example:

Screen.Recording.2023-08-03.at.14.53.06.mov

The curved taxi edge gets false positives way off the edge. You can compare to other edges, such as the haystacks in the video, to get a sense of how it should feel in proximity.

The corners need to be handled more accurately. For instance, you could

  1. use the centre of the curve's circle to precisely calculate the distance for the corner's hit test (i.e. using the inequality of the squared tap threshold and the squared distance to avoid the performance downside of using square roots); or
  2. make corner checks that mimic the curve with a reasonably-close bezier approximation -- using the existing math.inBezierVicinity() function; or
  3. replace the drawing of the corner of a curved taxi with a quadratic bezier -- and then you can use math.inBezierVicinity() with perfect accuracy (with the downside that it's not really a circle anymore).

If you try (2), temporarily using (3) would help to visualise your approximation even if you don't actually do (3) in the end result.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: Hit test code that is curved-taxi-specific should check edge.pstyle( 'curve-style' ).value === 'taxi', just like your drawing code, so that it's explicit and bounded.

const radius = edge.pstyle( 'taxi-radius' ).value/6
const isHor = pts[ i+1 ] === pts[ i + 3]
let x1,x2,y1,y2
if (isHor){
x1 = pts[ i ]
x2 = ( pts[ i ] === pts[ i + 2 ] ) ? pts[i+2] : ( pts[ i + 2 ] > pts[ i ] ) ? pts[ i + 2 ] - radius : pts[ i + 2 ] + radius;
y1 = pts[ i + 1] - radius
y2 = pts[ i + 3 ] + radius
}else{
x1 = pts[ i ] - radius
x2 = pts[ i + 2] + radius
y1 = pts[ i + 1]
y2 = ( pts[ i + 1 ] === pts[ i + 3 ] ) ? pts[ i + 1 ] : pts[ i + 3 ] > pts[ i+1 ] ? pts[ i + 3 ] - radius : pts[ i + 3 ] + radius;
}

if(
(math.inLineVicinity( x, y, x1, y1, x2, y2, width2 ))
){
addEle( edge, sqDist );
return true;
}
} else {
if(
(math.inLineVicinity( x, y, pts[ i ], pts[ i + 1], pts[ i + 2], pts[ i + 3], width2 ))
&&
widthSq > ( sqDist = math.sqdistToFiniteLine( x, y, pts[ i ], pts[ i + 1], pts[ i + 2], pts[ i + 3] ) )
){
addEle( edge, sqDist );
return true;
}
}
}

Expand Down
14 changes: 12 additions & 2 deletions src/extensions/renderer/canvas/drawing-edges.js
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,18 @@ CRp.drawEdgePath = function( edge, context, pts, type ){
case 'straight':
case 'segments':
case 'haystack':
Adam-Shea marked this conversation as resolved.
Show resolved Hide resolved
for( let i = 2; i + 1 < pts.length; i += 2 ){
context.lineTo( pts[ i ], pts[ i + 1] );
const taxiRadius= edge.pstyle( 'taxi-radius' )
const taxiRadiusValue = taxiRadius ? taxiRadius.pfValue : 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to how border-radius works in ordinary CSS, it would be useful to cap the taxi radius based on the available space. For instance, a div that's a 10px by 10px square can't have a radius greater than 5px effectively.

For the edges, the radius could be capped by the available space on the line.

If you have a larger radius, then the edge will bug out depending on the orientation of the nodes:

Screen.Recording.2023-08-03.at.15.29.33.mov

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N.b. this issue also effects smaller radii, but not as obviously.


if( edge.pstyle('curve-style').value == 'taxi' && taxiRadiusValue > 0 ) {
for( let i = 2; i + 1 < pts.length; i += 2 ) {
context.arcTo( pts[ i - 2 ], pts[ i - 1 ], pts[ i ], pts[ i + 1 ], taxiRadiusValue )
}
context.lineTo( pts[ pts.length - 2 ], pts[ pts.length - 1 ] );
Adam-Shea marked this conversation as resolved.
Show resolved Hide resolved
} else {
for( let i = 2; i + 1 < pts.length; i += 2 ) {
context.lineTo( pts[ i ], pts[ i + 1] );
}
Adam-Shea marked this conversation as resolved.
Show resolved Hide resolved
}
break;
}
Expand Down
4 changes: 3 additions & 1 deletion src/style/properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,7 @@ const styfn = {};
{ name: 'taxi-turn', type: t.bidirectionalSizeMaybePercent, triggersBounds: diff.any },
{ name: 'taxi-turn-min-distance', type: t.size, triggersBounds: diff.any },
{ name: 'taxi-direction', type: t.axisDirection, triggersBounds: diff.any },
{ name: 'taxi-radius', type: t.number, triggersBounds: diff.any },
Adam-Shea marked this conversation as resolved.
Show resolved Hide resolved
{ name: 'edge-distances', type: t.edgeDistances, triggersBounds: diff.any },
{ name: 'arrow-scale', type: t.positiveNumber, triggersBounds: diff.any },
{ name: 'loop-direction', type: t.angle, triggersBounds: diff.any },
Expand Down Expand Up @@ -682,6 +683,7 @@ styfn.getDefaultProperties = function(){
'taxi-turn': '50%',
'taxi-turn-min-distance': 10,
'taxi-direction': 'auto',
'taxi-radius': 0,
'edge-distances': 'intersection',
'curve-style': 'haystack',
'haystack-radius': 0,
Expand Down Expand Up @@ -777,4 +779,4 @@ styfn.addDefaultStylesheet = function(){
this.defaultLength = this.length;
};

export default styfn;
export default styfn;