-
Notifications
You must be signed in to change notification settings - Fork 433
Implement a CustomCombo node #7142
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,113 @@ | ||||||
| import { useChainCallback } from '@/composables/functional/useChainCallback' | ||||||
| import type { CanvasPointerEvent } from '@/lib/litegraph/src/types/events' | ||||||
| import { LGraphNode } from '@/lib/litegraph/src/LGraphNode' | ||||||
| import { LLink } from '@/lib/litegraph/src/litegraph' | ||||||
| import { app } from '@/scripts/app' | ||||||
|
|
||||||
| function applyToGraph(this: LGraphNode, extraLinks: LLink[] = []) { | ||||||
| if (!this.outputs[0].links?.length || !this.graph) return | ||||||
|
|
||||||
| const links = [ | ||||||
| ...this.outputs[0].links.map((l) => this.graph!.links[l]), | ||||||
| ...extraLinks | ||||||
| ] | ||||||
| let v = this.widgets?.[0].value | ||||||
| // For each output link copy our value over the original widget value | ||||||
| for (const linkInfo of links) { | ||||||
| const node = this.graph?.getNodeById(linkInfo.target_id) | ||||||
| const input = node?.inputs[linkInfo.target_slot] | ||||||
| if (!input) { | ||||||
| console.warn('Unable to resolve node or input for link', linkInfo) | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| const widgetName = input.widget?.name | ||||||
| if (!widgetName) { | ||||||
| console.warn('Invalid widget or widget name', input.widget) | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| const widget = node.widgets?.find((w) => w.name === widgetName) | ||||||
| if (!widget) { | ||||||
| console.warn(`Unable to find widget "${widgetName}" on node [${node.id}]`) | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| widget.value = v | ||||||
| widget.callback?.( | ||||||
| widget.value, | ||||||
| app.canvas, | ||||||
| node, | ||||||
| app.canvas.graph_mouse, | ||||||
| {} as CanvasPointerEvent | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
Comment on lines
+9
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Extract shared logic to reduce duplication. This function duplicates significant logic from Consider extracting the shared logic into a common utility function that both implementations can use, perhaps accepting a value-transformer function to handle the text-replacement difference. Example approach: // In a shared utility file
function propagateWidgetValue(
node: LGraphNode,
outputIndex: number,
value: any,
extraLinks: LLink[] = []
) {
if (!node.outputs[outputIndex].links?.length || !node.graph) return
const links = [
...node.outputs[outputIndex].links.map((l) => node.graph!.links[l]),
...extraLinks
]
for (const linkInfo of links) {
const targetNode = node.graph?.getNodeById(linkInfo.target_id)
const input = targetNode?.inputs[linkInfo.target_slot]
// ... rest of logic
}
}🤖 Prompt for AI Agents |
||||||
|
|
||||||
| function onNodeCreated(this: LGraphNode) { | ||||||
| this.applyToGraph = useChainCallback(this.applyToGraph, applyToGraph) | ||||||
|
|
||||||
| const comboWidget = this.widgets![0] | ||||||
| Object.defineProperty(comboWidget.options, 'values', { | ||||||
| get: () => { | ||||||
| return this.widgets!.filter( | ||||||
| (w) => w.name.startsWith('option') && w.value | ||||||
| ).map((w) => w.value) | ||||||
| } | ||||||
| }) | ||||||
| const options = comboWidget.options as { values: string[] } | ||||||
|
||||||
|
|
||||||
| function updateCombo() { | ||||||
| if (app.configuringGraph) return | ||||||
| const { values } = options | ||||||
| if (values.includes(`${comboWidget.value}`)) return | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Clarify string coercion intent. The comparison uses string template coercion If intentional, consider adding a comment: - if (values.includes(`${comboWidget.value}`)) return
+ // Coerce to string to handle mixed types
+ if (values.includes(`${comboWidget.value}`)) returnOr use explicit conversion for clarity: - if (values.includes(`${comboWidget.value}`)) return
+ if (values.includes(String(comboWidget.value))) return📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| comboWidget.value = values[0] ?? '' | ||||||
| } | ||||||
| comboWidget.callback = useChainCallback(comboWidget.callback, () => | ||||||
| this.applyToGraph!() | ||||||
| ) | ||||||
|
|
||||||
| function addOption(node: LGraphNode) { | ||||||
| if (!node.widgets) return | ||||||
| const widgetPostfix = | ||||||
| node.widgets | ||||||
| .findLast((w) => w.name.startsWith('option')) | ||||||
| ?.name?.slice(6) || '-1' | ||||||
| const newCount = parseInt(widgetPostfix) + 1 | ||||||
|
||||||
| const newCount = parseInt(widgetPostfix) + 1 | |
| const newCount = parseInt(widgetPostfix, 10) + 1 |
🤖 Prompt for AI Agents
In src/extensions/core/customCombo.ts around line 76, the parseInt call lacks a
radix; update the call to include the radix (e.g., parseInt(widgetPostfix, 10))
so the string is parsed as a base-10 integer, and if desired add a quick NaN
check after parsing to handle unexpected input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the custom value setter logic.
The custom getter/setter has complex side effects including:
- Calling
updateCombo()on every value change - Auto-adding new widgets when the last one is filled
- Auto-removing widgets with complex conditions
- Mutating the
node.widgetsarray during property access
This complexity makes the behavior harder to reason about and test. Consider:
- Extracting the auto-add/remove logic into separate, well-named functions
- Adding comments explaining the removal condition (line 95)
- Ensuring widget array mutations are safe if iteration is happening elsewhere
Example structure:
function shouldAutoAddOption(widget, value, node) {
return node.widgets.at(-1) === widget && value
}
function shouldAutoRemoveOption(widget, value, node) {
const lastWidget = node.widgets.at(-1)
const secondToLast = node.widgets.at(-2)
// Remove this widget if:
// - It has no value
// - AND it's second-to-last
// - AND the last widget also has no value
return !value && secondToLast === widget && !lastWidget?.value
}
set(v) {
value = v
updateCombo()
if (!node.widgets) return
if (shouldAutoAddOption(this, v, node)) {
addOption(node)
} else if (shouldAutoRemoveOption(this, v, node)) {
node.widgets.pop()
node.computeSize(node.size)
}
}🤖 Prompt for AI Agents
In src/extensions/core/customCombo.ts around lines 82 to 99, the custom setter
for widget.value mixes multiple side-effects (updateCombo, auto-add,
auto-remove) and mutates node.widgets inline, making behavior hard to reason
about and unsafe during iteration; refactor by extracting the auto-add and
auto-remove checks into two well-named helper functions (e.g.,
shouldAutoAddOption(widget, value, node) and shouldAutoRemoveOption(widget,
value, node)), replace the inline conditionals with calls to those helpers, add
a concise comment explaining the removal condition (why we remove the
second-to-last empty widget when the last is also empty), and make the mutation
of node.widgets safe (use the widget index to remove via splice or defer
mutation to a safe tick/event instead of mutating during property access).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Add defensive check for widgets array.
The code assumes
this.widgets[0]exists without validation. While the node definition likely ensures this, adding a defensive check improves robustness.📝 Committable suggestion
🤖 Prompt for AI Agents