Migrate parentIds when converting to subgraph #5708
Conversation
dosubot
bot
added
the
size:M
🎭 Playwright Test Results✅ All tests passed! ⏰ Completed at: 09/22/2025, 05:16:53 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
|
Often, the resolution to mutating inputs is to introduce an intermediate pure function that returns the new desired state.
Right? Another benefit of devolving the structure like this is that the logic can be tested in isolation and then composed. I feel like there's a way to handle reroutes as links under the hood, but I'd have to look into it further to really check. |
|
Appreciate the insight.
My understanding is that it was an explicit design goal for the new reroutes to be transparent to the linking logic. It's always safe to pretend they don't exist unless the code you're working with specifically involves reroutes. But that's more a question for @webfiltered, IIRC. |
Definitely by design. Reroutes are not links; the original code treating them as links was the cause of extreme misery and woe. They are virtual waypoints, and the new system treats them as such. Definitely room to improve the reroute code though; it had legacy constraints. |
There was a problem hiding this comment.
Off the top of my head, this looks like it will be corrupt if you convert to subgraph when using "part" of a reroute chain.
Scenarios I wanted to address a while back:
- A node with an input link that goes outside the subgraph + the "second" reroute only
The closest reroute is NOT selected - A node with an output link that goes outside, taking the first, third, and fifth reroutes
Skip the second and fourth reroutes - Two connected nodes, but only some / none of their reroutes
- v1: Just bring ALL the reroutes in regardless
- Pick only a reroute - not the node before or the node after
- v1: Stupid extreme edge case, just ignore it and pretend you didn't see it
I am aware this is awful.
| let child: SerialisableLLink | Reroute = linkData | ||
| let nextReroute = reroutes.get(child.parentId ?? 0) | ||
| while (child.parentId && nextReroute) { | ||
| child = nextReroute | ||
| nextReroute = reroutes.get(child.parentId ?? 0) | ||
| } | ||
| //set outside link to first remaining reroute OR undefined | ||
| link.parentId = child.parentId | ||
| //ensure end of chain is undefined | ||
| child.parentId = undefined |
There was a problem hiding this comment.
undefined is the idiomatic JS for "not set" / "none" - 0 is actually a valid reroute ID.
The use of 0 as a special value in LiteGraph is not something you see in libraries (for good reason - it allows you to use truthy checks, but breaks numeric checks). Even base JS uses -1 in its always-numeric built-ins.
There was a problem hiding this comment.
(Also thanks! Decided to make sure I wasn't losing my mind, and found a bad refactor of mine from Feb.)
There was a problem hiding this comment.
IT IS? lastRerouteId starts at 0, is incremented before use, and there's existing code sections that depend on valid rerouteIds always being truthy
src/LLink.ts
208: if (!linkSegment.parentId) return []
232: if (!linkSegment.parentId) return
501: if (this.parentId) copy.parentId = this.parentIdThis is one of the cases where js will happily let you index by undefined for correct behaviour, and I'm just hoping to satisfy typescript. It can be easily changed to a ternary if it feels ick to you.
Off the top of my head, this looks like it will be corrupt if you convert to subgraph when using "part" of a reroute chain.
Ran into a couple of these when trying to write test cases here. Yeah... it gets awful fast. If I'm following along right, 'ghost' reroutes (with no links) are not intended. A reroute that isn't linked through a parentId can be selected, but isn't drawn. There's some actions which can cause them, like a "Right click slot -> Disconnect Links".
Worthyness of implementation time aside, I'd expect a robust implementation to be a single rule that handles all these cases. Automatic inclusion seems both more UX friendly and seems to have the easier implementation of the 2.
- Walk the chain of parentIds for every link and reroute, only append chain if it reaches a selected item
- Assign each reroute a (stable) depth from the reversed parentId list. Sort and rebuild parentId chain from depth for each reroute
Worth editing to the unpacking code to console.error; continue'when a chain has an unexpected dead end? It shouldn't actually break anything, but feels like giving up.
There was a problem hiding this comment.
IT IS?
lastRerouteIdstarts at 0,
Yeah I forgot I did this - I -think- it was added in. I found it after I wrote the comment, but decided not to correct myself - there may be workflows from a couple versions that would use the old reroute ID 0 and cause unexpected behaviour.
The TL;DR is that the API design is still that ID 0 is valid - the current impl. detail is that it will not be on workflows created right now. There should be many more code paths that do nullish checks. So trying to have 0 also mean "not set" will result in -- what's the technical -- weird shit happening. 😃
(It looks like you were typing that response at the same time as I was doing my refactor - and I missed it~)
There was a problem hiding this comment.
'ghost' reroutes (with no links) are not intended
Yep - that's nasty.
Automatic inclusion seems both more UX friendly
SGTM!
Worth editing to the unpacking code to
console.error; continue'
Pretty much always worth logging unexpected state, imo. Actionable information beats silently breaking for me. Discarding error state is a pet hate of mine - it makes debugging impossible.
Spent a lot of time this year making sure LiteGraph absolutely spits the dummy in really obvious ways when things break. Throw, log, or.. do something. Initial work on subgraph had an insane difficulty spike at the start, due to this - tracing why a change seemed to do nothing just took forever (no errors, no warnings.. just.. didn't work).
Yeah... it gets awful fast.
Just QFT - sounds like a great tagline / subtitle.
Ah lost some other words that were meant to come before this. TL;DR would have been: Reroutes were put in the too-hard basket and this limitation was acknowledged somewhere. Sorry~ |
The parentId property on links and reroutes was not handled at all in the "Convert to Subgraph" code.
This needs to be addressed in 4 cases
This is handled in two parts by adding logic where the boundry links is created
Looks like litegraph tests aren't enabled and cursory glance shows multiple need to be updated to reflect recent changes. I'll still try to add some tests anyways.
EDIT: Tests are non functional. Seems the subgraph conversion call requires the rest of the frontend is running and has event listeners to register the subgraph node def. More work than anticipated, best revisited later
Resolves #5669
┆Issue is synchronized with this Notion page by Unito