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

Adam-Shea · 2023-07-24T15:40:24Z

Associated issues: This is a follow up to this discussion #3135

Added rounded corners to haystacks when the taxi-curve property is set to yes.

Happy to change property name or move elsewhere if you think there is a better format
Thanks :)

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.

maxkfranz

This looks like a good start, and I'm sure it would be mergeable with some iteration.

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

src/style/properties.js

maxkfranz · 2023-07-25T14:37:46Z

It would be useful to specify exactly how you propose your feature should be used. Give an example of how the style properties, old and new, should be specified in order to achieve your proposed effect. For instance, it's not clear why this would be coupled to haystack edges.

hassniazi · 2023-07-26T08:15:09Z

Great feedback, thanks @maxkfranz - we had already begun work to make it a configurable value, will change the property name and provide some documentation on how users could make use it.

maxkfranz · 2023-07-28T15:25:48Z

Great. We're planning on releasing 3.26.0 on next week's Friday, the 4th of August. It would be great to have this feature in the release.

Looking forward to your updates.

hassniazi · 2023-07-31T08:47:28Z

@maxkfranz - we've made most of the changes now to get this feature in. Just in the process of testing / fixing up, will get the PR updated shortly...

moved to taxi radius values

Adam-Shea · 2023-07-31T11:25:35Z

Hey, I've updated the pr with the suggestions you made.
Now when supplying taxi-radius, you can set a custom curve for your taxi corners.
I've fix the haystack removal, that was an issue with my merge sorry. Now this sits within haystack as taxi uses it by default. I have tested both with and without taxi-radius and taxi works just fine.
Happy to make anymore suggestions.

maxkfranz

Great. Getting close.

maxkfranz · 2023-08-01T12:43:32Z

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

- 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.value : 0


Suggested change

const taxiRadiusValue = taxiRadius ? taxiRadius.value : 0

const taxiRadiusValue = taxiRadius ? taxiRadius.pfValue : 0

Preferred value (pfValue, i.e. px), not raw value (value, e.g. 1em has value of 1)

maxkfranz · 2023-08-01T12:47:23Z

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

+ const taxiRadius= edge.pstyle( 'taxi-radius' )
+ const taxiRadiusValue = taxiRadius ? taxiRadius.value : 0
+
+ if( taxiRadiusValue > 0) {


This block should only apply to taxi edges, or the property should explicitly support all the affected cases.

Currently, it's breaking other edges. E.g. on the debug page with all edges set with taxi-radius: 32px:

Expected:

Actual:

src/style/properties.js

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

Curved taxi updated

hassniazi · 2023-08-03T08:20:51Z

We've pushed up the latest changes @maxkfranz - let us know if you need anything else

maxkfranz

Great. We're making progress. I've attached some comments below:

maxkfranz · 2023-08-03T18:43:57Z

documentation/md/style.md

- * 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`).


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`).

Just a clarification

maxkfranz · 2023-08-03T19:07:15Z

src/extensions/renderer/base/coord-ele-math/coords.js

- ){
- addEle( edge, sqDist );
- return true;
+ if( rs.edgeType === 'segments' && edge.pstyle( 'taxi-radius' ).value > 0 ) {


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

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

make corner checks that mimic the curve with a reasonably-close bezier approximation -- using the existing math.inBezierVicinity() function; or

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.

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.

maxkfranz · 2023-08-03T19:26:02Z

documentation/md/style.md

+ * 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`).
+


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.

maxkfranz · 2023-08-03T19:31:16Z

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

- 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


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

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

maxkfranz · 2023-08-16T14:09:22Z

@Adam-Shea @hassniazi, let us know if any of the feedback is unclear.

Looking forward to your updates

Adam-Shea · 2023-08-16T14:10:23Z

Heya, all clear! Just finding some time to get the last few changes in

stale · 2023-09-03T09:34:29Z

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-09-11T17:12:32Z

@Adam-Shea, are you still interested in this feature?

feature: curved taxi haystack

606714c

maxkfranz self-requested a review July 25, 2023 14:23

maxkfranz reviewed Jul 25, 2023

View reviewed changes

src/extensions/renderer/canvas/drawing-edges.js Show resolved Hide resolved

src/style/properties.js Outdated Show resolved Hide resolved

maxkfranz changed the title ~~feature: curved taxi haystack~~ Taxi edge turn radius (i.e. smooth curve on each turn) Jul 27, 2023

Adam Shea and others added 2 commits July 31, 2023 11:31

moved to taxi radius values

6c1644c

Merge pull request #1 from Adam-Shea/CurvedTaxiUpdated

e68f1af

moved to taxi radius values

maxkfranz requested changes Aug 1, 2023

View reviewed changes

Adam-Shea and others added 6 commits August 2, 2023 11:20

Update drawing-edges.js

a73a6c1

moved to pfValue

a95aa5b

only allow on taxi

2c825ec

added docs

acf2009

added better detection

61980eb

Merge pull request #2 from Adam-Shea/CurvedTaxiUpdated

92d7dc8

Curved taxi updated

maxkfranz self-requested a review August 3, 2023 18:43

maxkfranz requested changes Aug 3, 2023

View reviewed changes

stale bot added the stale label Sep 3, 2023

stale bot closed this Sep 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Adam-Shea commented Jul 24, 2023 •

edited

Loading

maxkfranz left a comment

maxkfranz commented Jul 25, 2023

hassniazi commented Jul 26, 2023 •

edited

Loading

maxkfranz commented Jul 28, 2023

hassniazi commented Jul 31, 2023

Adam-Shea commented Jul 31, 2023

maxkfranz left a comment

maxkfranz Aug 1, 2023

maxkfranz Aug 1, 2023

maxkfranz Aug 1, 2023 •

edited

Loading

hassniazi commented Aug 3, 2023

maxkfranz left a comment

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz Aug 3, 2023

maxkfranz commented Aug 16, 2023

Adam-Shea commented Aug 16, 2023

stale bot commented Sep 3, 2023

maxkfranz commented Sep 11, 2023

	const taxiRadiusValue = taxiRadius ? taxiRadius.value : 0
	const taxiRadiusValue = taxiRadius ? taxiRadius.pfValue : 0

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

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

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

Conversation

Adam-Shea commented Jul 24, 2023 • edited Loading

maxkfranz left a comment

Choose a reason for hiding this comment

maxkfranz commented Jul 25, 2023

hassniazi commented Jul 26, 2023 • edited Loading

maxkfranz commented Jul 28, 2023

hassniazi commented Jul 31, 2023

Adam-Shea commented Jul 31, 2023

maxkfranz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxkfranz Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

hassniazi commented Aug 3, 2023

maxkfranz left a 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

maxkfranz commented Aug 16, 2023

Adam-Shea commented Aug 16, 2023

stale bot commented Sep 3, 2023

maxkfranz commented Sep 11, 2023

Adam-Shea commented Jul 24, 2023 •

edited

Loading

hassniazi commented Jul 26, 2023 •

edited

Loading

maxkfranz Aug 1, 2023 •

edited

Loading