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

New edge type: smooth-segments #3201

Closed
4 tasks
EliotRagueneau opened this issue Dec 12, 2023 · 6 comments · Fixed by #3203
Closed
4 tasks

New edge type: smooth-segments #3201

EliotRagueneau opened this issue Dec 12, 2023 · 6 comments · Fixed by #3203
Labels
pinned A long-lived issue, such as a discussion
Milestone

Comments

@EliotRagueneau
Copy link
Contributor

EliotRagueneau commented Dec 12, 2023

Description of new feature
It would be awesome to internally support a new edge type: smooth segments.

I have a working implementation using unbundled bezier edges already.
You can see a demo of it at https://reactome-pwp.github.io/PathwayBrowser/R-HSA-453279
However, to do this, I have to define 3 points for each segment end point, which is not super user-friendly, specially when using a tool like https://github.com/iVis-at-Bilkent/cytoscape.js-edge-editing.

Also, at the moment, I'm using quadratic curves to do the rounding of the corners, but I think it would look even better if we could use arcs instead.

The principle I used so far is represented in the following schema
smooth-segments

In terms of added properties, I would consider adding the following:

  • curve-style: smooth-segments
  • control-point-distances : same as bezier
  • control-point-weights: same as bezier
  • control-point-radii: number | number[] | string
    • If one number is provided, apply it to all control points.
    • If more than one radius is provided, each radius is for a specific control point.
    • The string should be parsed like distances and weights are, and otherwise follow the same logic
  • smoothing-type: arc | quadratic

The code implementing it the following:

import {Position} from "cytoscape";
import {add, array, NDArray, subtract} from "vectorious";

/**
 * Add roundness to a segment edge by adding for each anchor position an additional point just before and just after
 * at a distance of radius from the original anchor point.
 *
 * With Cytoscape Bezier edges, this renders as a segmented edges with smoothed corner.
 * For a schematic explanation, @see https://raw.githubusercontent.com/reactome-pwp/PathwayBrowser/main/doc/smooth-segments.png
 *
 * @param source The position of the source node
 * @param target The position of the target node
 * @param anchors The intermediate segment points
 * @param radius How far away the new points need to be from the anchor points.
 * A bigger radius means smoother corners,
 * but it shouldn't be bigger than the segment
 * @private
 */
export function addRoundness(source: Position, target: Position, anchors: Position[], radius = 40): Position[] {
  const output: NDArray[] = []
  if (anchors.length === 0) return output;

  const points = [source, ...anchors, target];
  for (let i = 1; i < points.length - 1; i++) {
    let previousPoint = toVector(points[i - 1]);
    let currentPoint = toVector(points[i]);
    let nextPoint = toVector(points[i + 1]);

    const previousVector = subtract(previousPoint.copy(), currentPoint);
    const nextVector = subtract(nextPoint.copy(), currentPoint);

    // Scaling to fit to radius
    scaleTo([previousVector, nextVector], radius);

    previousPoint = add(currentPoint.copy(), previousVector);
    nextPoint = add(currentPoint.copy(), nextVector);

    output.push(previousPoint, currentPoint, nextPoint);
  }
  return output.map(toPosition);
}

/**
 * Scale a list of vectors to a desired length.
 *
 * If limit is true, limiting the resulting length to half the minimum norm of all vectors, in order to avoid clashing
 * @param vectors
 * @param length
 * @param limit
 * @private
 */
function scaleTo(vectors: NDArray[], length: number, limit = true): NDArray[] {
  const norms = vectors.map(vector => vector.norm());
  if (limit) {
    const minNorm = Math.min(...norms);
    if (length >= minNorm) length = minNorm / 2.1;
  }
  return vectors.map((vector, i) => vector.scale(length / norms[i]));
}


function toVector(position: Position): NDArray {
  return array([position.x, position.y]);
}

function toPosition(vector: NDArray): Position {
  return {x: vector.x, y: vector.y}
}

Motivation for new feature

We would like to use this new edge type to allow both the easy creation of smooth cornered segment edges, using https://github.com/iVis-at-Bilkent/cytoscape.js-edge-editing or something similar, and an easy visualisation of it without multiplying the number of user inputs. This would be used in the incoming reactome-cytoscape-style, of which you can see our current demo at https://reactome-pwp.github.io/PathwayBrowser/R-HSA-453279.

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

  • Ensure that the reporter has adequately described their idea. If not, elicit more information about the use case. You should iteratively build a spec together.
  • Ensure that the issue is a good fit for the core library. Some things are best done in extensions (e.g. UI-related features that aren't style-related). Some things are best done by app authors themselves -- instead of in Cytoscape libraries.
  • The issue has been associated with a corresponding milestone.
  • The commits have been incorporated into the unstable branch via pull request. The corresponding pull request is cross-referenced.
@maxkfranz
Copy link
Member

Description of new feature It would be awesome to internally support a new edge type: smooth segments.

I have a working implementation using unbundled bezier edges already. You can see a demo of it at https://reactome-pwp.github.io/PathwayBrowser/R-HSA-453279 However, to do this, I have to define 3 points for each segment end point, which is not super user-friendly, specially when using a tool like https://github.com/iVis-at-Bilkent/cytoscape.js-edge-editing.

Also, at the moment, I'm using quadratic curves to do the rounding of the corners, but I think it would look even better if we could use arcs instead.

Would either rounded corner type have problematic edge cases? Would they give visually-pleasing results for all angles?

The principle I used so far is represented in the following schema smooth-segments

This looks reasonable.

In terms of added properties, I would consider adding the following:

  • curve-style: smooth-segments

There are a number of other properties that use the term 'round' or 'rounded', so we should use that to be consistent, i.e. curve-style: round-segments -- like shape: round-rectangle.

  • control-point-distances : same as bezier
  • control-point-weights: same as bezier

Do you mean the same as segments? The bezier edges control points don't correspond to where an edge lies, they correspond to points towards which the curve is pulled. The segment points specify exactly where the line should hit.

  • control-point-radii: number | number[] | string

    • If one number is provided, apply it to all control points.

Good

  • If more than one radius is provided, each radius is for a specific control point.

Good. There should be a warning in the console if an insufficient number of radii are provided, as compared to the number of specified points. In that case, you could use the first value, the last value, or some default value -- just to show something so it's not completely broken with user error.

  • The string should be parsed like distances and weights are, and otherwise follow the same logic
  • smoothing-type: arc | quadratic

If you find that one type is superior to the other, let's just support that one to simplify things. If there are trade-offs, we should document them alongside this property.

I suggest a more explicit property name, like curve-rounding-style, so that it's clear it corresponds to curve-style: round-segments.

Something like smoothing-type, while a fairly good name, could be confused for things like nodes or background images.

The code implementing it the following:

import {Position} from "cytoscape";
import {add, array, NDArray, subtract} from "vectorious";

/**
 * Add roundness to a segment edge by adding for each anchor position an additional point just before and just after
 * at a distance of radius from the original anchor point.
 *
 * With Cytoscape Bezier edges, this renders as a segmented edges with smoothed corner.
 * For a schematic explanation, @see https://raw.githubusercontent.com/reactome-pwp/PathwayBrowser/main/doc/smooth-segments.png
 *
 * @param source The position of the source node
 * @param target The position of the target node
 * @param anchors The intermediate segment points
 * @param radius How far away the new points need to be from the anchor points.
 * A bigger radius means smoother corners,
 * but it shouldn't be bigger than the segment
 * @private
 */
export function addRoundness(source: Position, target: Position, anchors: Position[], radius = 40): Position[] {
  const output: NDArray[] = []
  if (anchors.length === 0) return output;

  const points = [source, ...anchors, target];
  for (let i = 1; i < points.length - 1; i++) {
    let previousPoint = toVector(points[i - 1]);
    let currentPoint = toVector(points[i]);
    let nextPoint = toVector(points[i + 1]);

    const previousVector = subtract(previousPoint.copy(), currentPoint);
    const nextVector = subtract(nextPoint.copy(), currentPoint);

    // Scaling to fit to radius
    scaleTo([previousVector, nextVector], radius);

    previousPoint = add(currentPoint.copy(), previousVector);
    nextPoint = add(currentPoint.copy(), nextVector);

    output.push(previousPoint, currentPoint, nextPoint);
  }
  return output.map(toPosition);
}

/**
 * Scale a list of vectors to a desired length.
 *
 * If limit is true, limiting the resulting length to half the minimum norm of all vectors, in order to avoid clashing
 * @param vectors
 * @param length
 * @param limit
 * @private
 */
function scaleTo(vectors: NDArray[], length: number, limit = true): NDArray[] {
  const norms = vectors.map(vector => vector.norm());
  if (limit) {
    const minNorm = Math.min(...norms);
    if (length >= minNorm) length = minNorm / 2.1;
  }
  return vectors.map((vector, i) => vector.scale(length / norms[i]));
}


function toVector(position: Position): NDArray {
  return array([position.x, position.y]);
}

function toPosition(vector: NDArray): Position {
  return {x: vector.x, y: vector.y}
}

Motivation for new feature

We would like to use this new edge type to allow both the easy creation of smooth cornered segment edges, using https://github.com/iVis-at-Bilkent/cytoscape.js-edge-editing or something similar, and an easy visualisation of it without multiplying the number of user inputs. This would be used in the incoming reactome-cytoscape-style, of which you can see our current demo at https://reactome-pwp.github.io/PathwayBrowser/R-HSA-453279.

For reviewers

Reviewers should ensure that the following tasks are carried out for incorporated issues:

  • Ensure that the reporter has adequately described their idea. If not, elicit more information about the use case. You should iteratively build a spec together.
  • Ensure that the issue is a good fit for the core library. Some things are best done in extensions (e.g. UI-related features that aren't style-related). Some things are best done by app authors themselves -- instead of in Cytoscape libraries.
  • The issue has been associated with a corresponding milestone.
  • The commits have been incorporated into the unstable branch via pull request. The corresponding pull request is cross-referenced.

@maxkfranz
Copy link
Member

@EliotRagueneau, I've added some specific comments in the previous post in this thread.

The feature looks good.

Let me know if you have any questions re. design decisions or re. implementation. If everything is clear, then I think this should just be merged in.

@EliotRagueneau
Copy link
Contributor Author

EliotRagueneau commented Dec 15, 2023

I've observed that taxi edges are internally using segments, and while testing the round-segment edges, it also got applied to taxi edges, and it looks great in my opinion.

round-taxi

This made me think that maybe instead of adding a new curve-style, this"round" effect could be applied any time a radius is provided to any curve-style that has sharp corners, more like a property of the already existing styles than a new style on its own.

The alternative would be to also create a round-taxi cruve style. Which option do you prefer?

Debug with all round taxi edges

@maxkfranz
Copy link
Member

It's better from a user's perspective to have separate, local shape enums (e.g. round-taxi), rather than a global one (e.g. edge-join: round).

(1) Separate, local shapes are consistent with existing shapes, e.g. round-rectangle. Consistency facilitates learning.

(2) Not all curve styles would support being rounded. Having a separate property (e.g. edge-join) would create opportunities for user error -- and disappointment.

@EliotRagueneau
Copy link
Contributor Author

Okay, I'll add both round-taxi and round-segments then

@EliotRagueneau EliotRagueneau mentioned this issue Dec 15, 2023
7 tasks
Copy link

stale bot commented Dec 29, 2023

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.

@stale stale bot added the stale label Dec 29, 2023
@maxkfranz maxkfranz added pinned A long-lived issue, such as a discussion and removed stale labels Jan 5, 2024
@maxkfranz maxkfranz linked a pull request Jan 24, 2024 that will close this issue
7 tasks
@maxkfranz maxkfranz added this to the 3.29.0 milestone Jan 24, 2024
@EliotRagueneau EliotRagueneau mentioned this issue Feb 1, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned A long-lived issue, such as a discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants