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

Feature/line outline #3244

Merged
merged 4 commits into from
May 24, 2024
Merged

Conversation

xkdgo
Copy link
Contributor

@xkdgo xkdgo commented May 16, 2024

Associated issues:

Notes re. the content of the pull request. Give context to reviewers or serve as a general record of the changes made. Add a screenshot or video to demonstrate your new feature, if possible.

Demo with outlines
line-outline-demo.zip

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.

@mikekucera
Copy link
Contributor

mikekucera commented May 16, 2024

Hi, thanks for the pull request.

I have one concern,

This screenshot shows the styles from the demo, the line-outline-width is set to 2 and the underlay has a padding of 3px. You can see that the underlay is slightly wider than the outline (as expected).
Screenshot 2024-05-16 at 9 53 47 AM

However, when I set the underlay padding to 2px it looks like below. If the outline and the underlay are both set to 2, shouldn't they line up perfectly? I think It should be possible to make the underlay and outline have the exact same width so that they match.
Screenshot 2024-05-16 at 9 39 53 AM

@xkdgo
Copy link
Contributor Author

xkdgo commented May 16, 2024

It should be possible to make the underlay and outline have the exact same width so that they match.

To test this i changed line's width calculation
from

context.lineWidth = edgeWidth + lineOutlineWidth;

to

context.lineWidth = edgeWidth + lineOutlineWidth / 2;

line-outline-demo-w.zip
Screenshot from 2024-05-16 22-17-18

@xkdgo
Copy link
Contributor Author

xkdgo commented May 17, 2024

@mikekucera I was wrong when I changed line's width calculation.

It should be possible to make the underlay and outline have the exact same width so that they match.

But why we should do it ?

lets compare our width calculations
Underlay width calculate as double of its padding

    let padding = edge.pstyle(`${overlayOrUnderlay}-padding`).pfValue;
    let width = 2 * padding;

Result line width calculated either
edge's width if our outline is zero
or edgeWidth + lineOutlineWidth if lineOutlineWidth > 0. After commit 6ad45bf calculation changed to edgeWidth + lineOutlineWidth / 2.

Now when we set in demo edge's width to 3, and outline and the underlay are both set to 2, we will get the following values

underlay-width = 2 * 2 = 4
edge's width = 3 + 2 / 2 = 4

In this case we got equal values.
But if we change some of these values, we will no longer get a match, or we will get it by accident, or we will simply count the values ​​so that they coincide.

In my opinion underlay property serves to highlight a line. But line outline serves to show a line when line-color and the color of the background match. And dimensions of underlay should always be greater than edge's width with or not with outline.
Its up to user to control it.

I will be grateful for any advice on how to improve something in the PR.

@mikekucera
Copy link
Contributor

mikekucera commented May 17, 2024

I think you're right.
How about we treat the total width of the line as: line width + outline width, then the underlay padding should be in addition to the total width. That way to make the outline and underlay line up perfectly the underlay padding needs to be set to 0.

It already works this way with just dashed line and underlay. Here the underlay padding is set to 0...

Screenshot 2024-05-17 at 9 15 51 AM

But when the outline is turned on it should look like below...
The total edge width is greater, but the underlay still matches because the padding is 0.

Screenshot 2024-05-17 at 9 23 11 AM

What do you think, does that make sense?

@xkdgo
Copy link
Contributor Author

xkdgo commented May 17, 2024

When underlay-padding is 0 I think it should be switched off ?
May be when we have dashed line we can see underlay line but it swittched off. May be its a bug and not a feature ?

@xkdgo
Copy link
Contributor Author

xkdgo commented May 17, 2024

How about we treat the total width of the line as: line width + outline width

Could this confuse the user?
When user tuning outline, it simple to tune dimentions when he know width of his line, and he add some outline with color.

But of course it is possible to subtract the thickness of the outer line from the total line width. But in the library outline of nodes does not have same behaviour. We can check it when set outline-width : The size of the node’s outline.

@mikekucera
Copy link
Contributor

You're right again, node underlay padding does not appear to take the node border width or outline into consideration at all. I find this strange because the node's bounding box does appear to include the border. But in any case the way padding and width are related should be consistent with node borders.

I'm going to see if Max is available to weigh in on this. But so far it looks like your original PR is good. Thanks.

@xkdgo
Copy link
Contributor Author

xkdgo commented May 17, 2024

@mikekucera before the next discussion I returned the calculation to the original. Thank you.

@mikekucera mikekucera merged commit 42516fb into cytoscape:unstable May 24, 2024
@xkdgo xkdgo deleted the feature/lineOutline branch May 27, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants