Skip to content

Add the style of right-angle grid-aligned wires in the graph#2182

Merged
Keavon merged 14 commits into
GraphiteEditor:masterfrom
Stargazer10101:Snake-wires-node-graph
Feb 13, 2025
Merged

Add the style of right-angle grid-aligned wires in the graph#2182
Keavon merged 14 commits into
GraphiteEditor:masterfrom
Stargazer10101:Snake-wires-node-graph

Conversation

@Stargazer10101

@Stargazer10101 Stargazer10101 commented Jan 9, 2025

Copy link
Copy Markdown
Contributor

This PR represents an initial attempt to address [closes #2170], specifically focusing implementing the first part of the issue :
The wires follow only right-angle horizontal and vertical paths.
The corners have been rounded.

Remaining work:
Producing a final algorithm that avoids overlapping nodes and other wires given any positioning of nodes (similar to the high-fidelity node graph UI mock up posted with the issue).

Screenshot of current implementation:
Screenshot from 2025-01-09 20-05-09

@Keavon

Keavon commented Jan 9, 2025

Copy link
Copy Markdown
Member

This looks awesome! I think the easiest low-hanging fruit change is ensuring all wires line up with the grid, so vertical sections that are halfway between an odd-number of grid spaces apart should just round up.

Then for developing the algorithm further to be "smart", I imagine the first and most crucial step is a system for coordinating between wires to avoid occupying the same vertical or horizontal run (i.e. wires overlapping for some distance, meaning it's hard to see which is which) as much as possible by choosing to bend at different places so they can run parallel instead of on top of each other.

I'll need to play with it but, assuming the overlapping wires isn't too prevalent now that they don't have curvy bends, it probably makes sense to merge this PR first and then continue adding the routing algorithms in a subsequent PR. I will aim to review this PR today or tomorrow for you. Thanks, and welcome!

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

!build

@Keavon Keavon changed the title First iteration for [#2170] - Feedback Requested Right-angle wires in the node graph Jan 10, 2025
@github-actions

Copy link
Copy Markdown
📦 Build Complete for 5cfbbd3
https://2e1bbda1.graphite.pages.dev

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

One issue that I'm spotting now which should be quick to fix: your implementation seems to be giving the top output from a layer stack a thick wire, when it should be thin like it is in the current version.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

Thanks for feedback @Keavon . I'm looking into this.

One issue that I'm spotting now which should be quick to fix: your implementation seems to be giving the top output from a layer stack a thick wire, when it should be thin like it is in the current version.

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

Also one more observation: if you look very closely, the way your vertical wires line up with the grid (when the spacing is an even number), you will notice that the vertical line goes entirely to the right of the grid dots when it should straddle the grid evenly. So your current implementation has it drawing at a half-line width to the right.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

One issue that I'm spotting now which should be quick to fix: your implementation seems to be giving the top output from a layer stack a thick wire, when it should be thin like it is in the current version.

This was done so as to exactly match the UI mockup shown in the issue.
This is what it looks like otherwise:
Screenshot from 2025-01-10 10-21-38

Which is not identical with this(the mockup):
image

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

Also one more observation: if you look very closely, the way your vertical wires line up with the grid (when the spacing is an even number), you will notice that the vertical line goes entirely to the right of the grid dots when it should straddle the grid evenly. So your current implementation has it drawing at a half-line width to the right.

Working on this.

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

The mockup contains that tapered triangular portion at the base of the vertical wire when it exits the layer, but the actual wire itself is still a thin wire because it represents non-stacking data. Eventually I will get around to adding that triangular taper but that is just a stylistic element.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

Also one more observation: if you look very closely, the way your vertical wires line up with the grid (when the spacing is an even number), you will notice that the vertical line goes entirely to the right of the grid dots when it should straddle the grid evenly. So your current implementation has it drawing at a half-line width to the right.

I have made some changes. This is how it looks now:
Screenshot from 2025-01-10 12-35-28

Requesting your feedback.

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

That looks good, thanks. I'd be happy to merge this after the rounding of odd numbered horizontal grid spaces to avoid vertical lines halfway through the grid lines (like in your bottom center vertical line in your latest screenshot). And after the situation is resolved where a node is brought from being right of its predecessor to instead being to the left, where the wires will need to deal with cases 5 and 6 in the diagram in the original issue.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

I've made some changes. The vertical lines now lie on the grid lines. The cases 5 and 6 of the diagram are demonstrated in the bottom left of the following image:
Screenshot from 2025-01-10 14-23-58

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

!build

@Stargazer10101 Stargazer10101 marked this pull request as ready for review January 10, 2025 11:02
@github-actions

Copy link
Copy Markdown
📦 Build Complete for d7744dc
https://893aba6f.graphite.pages.dev

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

Nice work on case 6. Case 5 still needs to be implemented:
capture

@Keavon

Keavon commented Jan 10, 2025

Copy link
Copy Markdown
Member

There is also a zone where this isn't working as desired, but it works further to the left and further to the right. Basically, case 6 should kick in earlier instead of letting the port's output get cut off. Demonstrated in this video:

capture_2_.mp4

@Stargazer10101

Stargazer10101 commented Jan 12, 2025

Copy link
Copy Markdown
Contributor Author

I've made some changes. For the case 5 implementation was simple. For the zones where the wires behaved weirdly, I wrote a few cases (where the weird behavior occurs) individually.
The case 5 implementation has been shown on the top right of the screenshot below.
The implementations for other zones have been solved as shown in the bottom left of the screenshot below.
The wires follow a path so that they lie exactly on the grid lines, but let me know if you want them to follow a different path in a particular case.
Feedback requested.
Screenshot from 2025-01-12 21-47-28

@Keavon

Keavon commented Jan 12, 2025

Copy link
Copy Markdown
Member

!build

@Keavon Keavon changed the title Right-angle wires in the node graph Right-angle grid-aligned wires in the node graph Jan 12, 2025
@github-actions

Copy link
Copy Markdown
📦 Build Complete for c576ab7
https://1b25d2df.graphite.pages.dev

@Keavon

Keavon commented Jan 12, 2025

Copy link
Copy Markdown
Member

Looking a lot closer! Here are a few more edge cases:

  • Shouldn't go through its own node:

capture
capture
capture
capture

  • Shouldn't turn immediately up/down from the connector socket, it should instead pop out from the node/layer area by at least one grid cell:

capture
capture
capture

@Keavon

Keavon commented Jan 13, 2025

Copy link
Copy Markdown
Member

Marking this as a draft for now while awaiting changes. Please mark it as ready for review when you've solved them so I know to come back to this PR. Thanks!

@Stargazer10101 Stargazer10101 marked this pull request as ready for review January 16, 2025 09:46
@Keavon Keavon force-pushed the master branch 2 times, most recently from 7465fad to ab724d8 Compare January 18, 2025 00:03
@Keavon

Keavon commented Jan 19, 2025

Copy link
Copy Markdown
Member

!build

@Keavon

Keavon commented Jan 19, 2025

Copy link
Copy Markdown
Member

(Status: awaiting a considerable code consolidation refactor to reduce the amount of code 5-10x as discussed on Discord)

@github-actions

Copy link
Copy Markdown
📦 Build Complete for ab5fa92
https://edddf1fb.graphite.pages.dev

@Keavon Keavon marked this pull request as draft January 19, 2025 10:03
@Keavon

Keavon commented Jan 25, 2025

Copy link
Copy Markdown
Member

Hi @Stargazer10101, I was just checking in to see if you have any updates or concerns.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

Hey @Keavon , I couldn't find time to work on this until yesterday. Sorry about that. I've started working on this. Will keep you updated in the discord.

@Keavon

Keavon commented Jan 26, 2025

Copy link
Copy Markdown
Member

Thanks for the update.

@Keavon Keavon force-pushed the Snake-wires-node-graph branch from 8b94dec to f23f7ab Compare February 13, 2025 07:10
@Keavon Keavon marked this pull request as ready for review February 13, 2025 07:30
@Keavon

Keavon commented Feb 13, 2025

Copy link
Copy Markdown
Member

I was able to remove several hundred lines by consolidating the line corner definitions. Take a look at the diff if you're interested. I also found one more failing edge case that I hope you may be able to solve in a followup PR:

capture

The expected shape would be:

capture

I'm merging this now since it deserves to see the light of day. Thank you!

@Keavon Keavon changed the title Right-angle grid-aligned wires in the node graph Add the style of right-angle grid-aligned wires in the graph Feb 13, 2025
@Keavon Keavon enabled auto-merge (squash) February 13, 2025 07:45
@Keavon Keavon merged commit 26fa8d9 into GraphiteEditor:master Feb 13, 2025
@Keavon

Keavon commented Feb 13, 2025

Copy link
Copy Markdown
Member

Please also leave a comment with this PR number in #2170 for tracking purposes.

@Stargazer10101

Copy link
Copy Markdown
Contributor Author

I was able to remove several hundred lines by consolidating the line corner definitions. Take a look at the diff if you're interested. I also found one more failing edge case that I hope you may be able to solve in a followup PR:

capture

The expected shape would be:

capture

I'm merging this now since it deserves to see the light of day. Thank you!

I'll surely fix the newest failing edge case and open a PR. Also looking into the diff. Thank you for helping and I'm sorry you had to do this yourself. I'll continue to contribute wherever I can.

@Keavon

Keavon commented Feb 13, 2025

Copy link
Copy Markdown
Member

Thanks. Please also don't forget to leave the comment in the issue.

@Keavon

Keavon commented Jun 13, 2026

Copy link
Copy Markdown
Member

Hi @Stargazer10101, thanks again for this code contribution to the project! We're still hoping you will respond to the request to relicense this code. Please see #4208 ASAP, thank you!

@Keavon

Keavon commented Jun 15, 2026

Copy link
Copy Markdown
Member

Hi @Stargazer10101, we're grateful of your past code contribution to Graphite, but unless we hear from you by the end of this weekend with agreement about relicensing your code to include the MIT license, we will begin steps to remove your code contributions so that you will no longer be a Graphite code contributor. We'd much rather avoid this outcome, so please respond now.

To agree, simply log into your GitHub account, visit #4208, and copy-paste this into a comment:

"I license my past and future contributions to Graphite under the dual MIT/Apache-2.0 license, allowing licensees to choose either at their option."

If you're hesitant because you disagree or have questions, feel free to respond with another message; we'll do our best to answer your questions or concerns. More explanation is at the top of the linked thread if you're confused. So far, 164 contributors have agreed and you are among the 11% remaining whom we are still hoping to hear from before we begin removing your code from the project starting this Monday. (If you only see this later than Monday, your future belated response will still be appreciated.)

Thank you for your cooperation and support of open source, and we hope you'll help us keep your valued contributions in-tact.

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.

Snake-like wires in the node graph

2 participants