mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
fix: tighten ECS architecture docs and migration gates
Amp-Thread-ID: https://ampcode.com/threads/T-019d22e0-5d0a-767a-92c5-84ad5f2a4d00 Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
DrJKL
@@ -401,20 +401,18 @@ sequenceDiagram
|
||||
CS->>W: query Connectivity + SlotConnection for selected nodes
|
||||
CS->>CS: classify links as internal vs boundary
|
||||
|
||||
CS->>W: create new GraphId scope in scopes registry
|
||||
CS->>W: createEntity(SubgraphEntityId)
|
||||
CS->>W: setComponent(sgId, SubgraphMeta, { name: 'New Subgraph' })
|
||||
|
||||
Note over CS,W: Move selected entities into subgraph scope
|
||||
|
||||
CS->>W: setComponent(sgId, SubgraphStructure, {<br/> nodeIds: [...selected],<br/> linkIds: [...internal],<br/> rerouteIds: [...internal]<br/>})
|
||||
|
||||
Note over CS,W: Create SubgraphNode entity in parent scope
|
||||
|
||||
CS->>W: createEntity(NodeEntityId) [the SubgraphNode]
|
||||
CS->>W: setComponent(nodeId, Position, { center of selection })
|
||||
CS->>W: setComponent(nodeId, SubgraphStructure, { graphId, interface })
|
||||
CS->>W: setComponent(nodeId, SubgraphMeta, { name: 'New Subgraph' })
|
||||
|
||||
Note over CS,W: Re-parent selected entities into new graph scope
|
||||
|
||||
loop each selected entity
|
||||
CS->>W: update graphScope to new graphId
|
||||
end
|
||||
CS->>W: setComponent(nodeId, NodeType, { type: sgId })
|
||||
|
||||
Note over CS,W: Create boundary slots on SubgraphNode
|
||||
|
||||
@@ -433,7 +431,7 @@ sequenceDiagram
|
||||
|
||||
| Aspect | Current | ECS |
|
||||
| -------------------------- | ------------------------------------------------- | ------------------------------------------------------- |
|
||||
| Entity movement | Clone → serialize → configure → remove originals | Re-parent entities: update graphScope to new GraphId |
|
||||
| Entity movement | Clone → serialize → configure → remove originals | Move entity IDs into SubgraphStructure component |
|
||||
| Boundary links | Disconnect → remove → recreate → reconnect | Update LinkEndpoints to point at new SubgraphNode slots |
|
||||
| Intermediate inconsistency | Graph is partially disconnected during operation | Atomic: all component writes happen together |
|
||||
| Code size | 200+ lines | ~50 lines in system |
|
||||
@@ -500,27 +498,26 @@ sequenceDiagram
|
||||
|
||||
Caller->>CS: unpackSubgraph(world, subgraphNodeId)
|
||||
|
||||
CS->>W: getComponent(subgraphNodeId, SubgraphStructure)
|
||||
W-->>CS: { graphId, interface }
|
||||
CS->>W: getComponent(subgraphNodeId, NodeType)
|
||||
W-->>CS: { type: subgraphEntityId }
|
||||
|
||||
CS->>W: query entities where graphScope = graphId
|
||||
W-->>CS: all child entities (nodes, links, reroutes, etc.)
|
||||
CS->>W: getComponent(subgraphEntityId, SubgraphStructure)
|
||||
W-->>CS: { nodeIds, linkIds, rerouteIds }
|
||||
|
||||
Note over CS,W: Re-parent entities to containing graph scope
|
||||
Note over CS,W: Move entities back to parent scope
|
||||
|
||||
loop each child entity
|
||||
CS->>W: update graphScope to parent scope
|
||||
end
|
||||
CS->>W: remove nodeIds from SubgraphStructure
|
||||
Note over CS,W: Entities already exist in World — just reparent
|
||||
|
||||
Note over CS,W: Reconnect boundary links
|
||||
|
||||
loop each boundary slot in interface
|
||||
loop each boundary slot on SubgraphNode
|
||||
CS->>W: getComponent(slotId, SlotConnection)
|
||||
CS->>W: update LinkEndpoints: SubgraphNode slot → internal node slot
|
||||
end
|
||||
|
||||
CS->>W: deleteEntity(subgraphNodeId)
|
||||
CS->>W: remove graphId from scopes registry
|
||||
CS->>W: deleteEntity(subgraphEntityId) [if no other refs]
|
||||
|
||||
Note over CS,W: Offset positions
|
||||
|
||||
@@ -531,13 +528,13 @@ sequenceDiagram
|
||||
|
||||
### Key Differences
|
||||
|
||||
| Aspect | Current | ECS |
|
||||
| ----------------- | --------------------------------------------------- | --------------------------------------------------------------- |
|
||||
| ID remapping | `nodeIdMap[oldId] = newId` for every node and link | No remapping — entities keep their IDs, only graphScope changes |
|
||||
| Magic IDs | SUBGRAPH_INPUT_ID = -10, SUBGRAPH_OUTPUT_ID = -20 | No magic IDs — boundary modeled as slot entities |
|
||||
| Clone vs move | Clone nodes, assign new IDs, configure from scratch | Move entity references between scopes |
|
||||
| Link reconnection | Remap origin_id/target_id, create new LLink objects | Update LinkEndpoints component in place |
|
||||
| Complexity | ~200 lines with deduplication and reroute remapping | ~40 lines, no remapping needed |
|
||||
| Aspect | Current | ECS |
|
||||
| ----------------- | --------------------------------------------------- | ------------------------------------------------ |
|
||||
| ID remapping | `nodeIdMap[oldId] = newId` for every node and link | No remapping — entities keep their IDs |
|
||||
| Magic IDs | SUBGRAPH_INPUT_ID = -10, SUBGRAPH_OUTPUT_ID = -20 | No magic IDs — boundary modeled as slot entities |
|
||||
| Clone vs move | Clone nodes, assign new IDs, configure from scratch | Move entity references between scopes |
|
||||
| Link reconnection | Remap origin_id/target_id, create new LLink objects | Update LinkEndpoints component in place |
|
||||
| Complexity | ~200 lines with deduplication and reroute remapping | ~40 lines, no remapping needed |
|
||||
|
||||
## 6. Connect Slots
|
||||
|
||||
|
||||
@@ -10,14 +10,6 @@ target architecture, see [ECS Target Architecture](ecs-target-architecture.md).
|
||||
For verified accuracy of these documents, see
|
||||
[Appendix: Critical Analysis](appendix-critical-analysis.md).
|
||||
|
||||
## Planning assumptions
|
||||
|
||||
- The bridge period is expected to span 2-3 release cycles.
|
||||
- Bridge work is treated as transitional debt with explicit owners and sunset
|
||||
checkpoints, not as a permanent architecture layer.
|
||||
- Phase 5 is entered only by explicit go/no-go review against the criteria in
|
||||
this document.
|
||||
|
||||
## Phase 0: Foundation
|
||||
|
||||
Zero behavioral risk. Prepares the codebase for extraction without changing
|
||||
@@ -96,11 +88,7 @@ Define branded types in a new `src/ecs/entityId.ts`:
|
||||
```
|
||||
type NodeEntityId = number & { readonly __brand: 'NodeEntityId' }
|
||||
type LinkEntityId = number & { readonly __brand: 'LinkEntityId' }
|
||||
type WidgetEntityId = number & { readonly __brand: 'WidgetEntityId' }
|
||||
type SlotEntityId = number & { readonly __brand: 'SlotEntityId' }
|
||||
type RerouteEntityId = number & { readonly __brand: 'RerouteEntityId' }
|
||||
type GroupEntityId = number & { readonly __brand: 'GroupEntityId' }
|
||||
type GraphId = string & { readonly __brand: 'GraphId' } // scope, not entity
|
||||
// ... etc per ADR 0008
|
||||
```
|
||||
|
||||
Add cast helpers (`asNodeEntityId(id: number): NodeEntityId`) for use at
|
||||
@@ -158,7 +146,7 @@ interface World {
|
||||
slots: Map<SlotEntityId, SlotComponents>
|
||||
reroutes: Map<RerouteEntityId, RerouteComponents>
|
||||
groups: Map<GroupEntityId, GroupComponents>
|
||||
scopes: Map<GraphId, GraphId | null> // graph scope DAG (parent or null for root)
|
||||
subgraphs: Map<SubgraphEntityId, SubgraphComponents>
|
||||
|
||||
createEntity<K extends EntityKind>(kind: K): EntityIdFor<K>
|
||||
deleteEntity<K extends EntityKind>(kind: K, id: EntityIdFor<K>): void
|
||||
@@ -167,15 +155,6 @@ interface World {
|
||||
}
|
||||
```
|
||||
|
||||
Subgraphs are not a separate entity kind. A node with a `SubgraphStructure`
|
||||
component represents a subgraph. The `scopes` map tracks the graph nesting DAG.
|
||||
See [Subgraph Boundaries](subgraph-boundaries-and-promotion.md) for the full
|
||||
model.
|
||||
|
||||
World scope is per workflow instance. Linked subgraph definitions can be reused
|
||||
across instances, but mutable runtime state (widget values, execution state,
|
||||
selection/transient view state) remains instance-scoped through `graphId`.
|
||||
|
||||
Initial implementation: plain `Map`-backed. No reactivity, no CRDT, no
|
||||
persistence. The World exists but nothing populates it yet.
|
||||
|
||||
@@ -259,18 +238,6 @@ A bridge can move from "transitional" to "removal candidate" only when:
|
||||
|
||||
These criteria prevent the bridge from becoming permanent by default.
|
||||
|
||||
### Bridge duration and maintenance controls
|
||||
|
||||
To contain dual-path maintenance cost during Phases 2-4:
|
||||
|
||||
- Every bridge concern has a named owner and target sunset release.
|
||||
- Every PR touching bridge-covered data paths must include parity tests for both
|
||||
legacy and World-driven execution.
|
||||
- Bridge fallback usage is instrumented in integration/e2e and reviewed every
|
||||
milestone; upward trends block new bridge expansion.
|
||||
- Any bridge that misses its target sunset release requires an explicit risk
|
||||
review and revised removal plan.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3: Systems
|
||||
@@ -378,33 +345,6 @@ source of truth; WidgetValueStore becomes a read-through cache or is removed.
|
||||
**Risk:** Medium. WidgetValueStore is already well-abstracted. The main
|
||||
change is routing writes through the World instead of the store.
|
||||
|
||||
### 4d. Layout write path and render decoupling
|
||||
|
||||
Remove layout side effects from render incrementally by node family.
|
||||
|
||||
**Approach:**
|
||||
|
||||
1. Inventory `drawNode()` call paths that still trigger `arrange()`.
|
||||
2. For one node family at a time, run `LayoutSystem` in update phase and mark
|
||||
entities as layout-clean before render.
|
||||
3. Keep a temporary compatibility fallback that runs legacy layout only for
|
||||
non-migrated families.
|
||||
4. Delete fallback once parity tests and frame-time budgets are met.
|
||||
|
||||
**Risk:** High. Mixed-mode operation must avoid stale layout reads. Requires
|
||||
family-level rollout and targeted regression tests.
|
||||
|
||||
### Render hot-path performance gate
|
||||
|
||||
Before enabling ECS render reads as default for any migrated family:
|
||||
|
||||
- Benchmark representative workflows (200-node and 500-node minimum).
|
||||
- Compare legacy vs ECS p95 frame time and mean draw cost.
|
||||
- Block rollout on statistically significant regression beyond agreed budget
|
||||
(default budget: 5% p95 frame-time regression ceiling).
|
||||
- Capture profiler traces proving the dominant cost is not repeated
|
||||
`world.getComponent()` lookups.
|
||||
|
||||
### Phase 3 -> 4 gate (required)
|
||||
|
||||
Phase 4 starts only when all of the following are true:
|
||||
@@ -418,10 +358,6 @@ Phase 4 starts only when all of the following are true:
|
||||
- A representative extension suite passes, including `rgthree-comfy`.
|
||||
- Write bridge re-entrancy tests prove there is no World <-> legacy feedback
|
||||
loop.
|
||||
- Layout migration for any enabled node family passes read-only render checks
|
||||
(no `arrange()` writes during draw).
|
||||
- Render hot-path benchmark gate passes for every family moving to ECS-first
|
||||
reads.
|
||||
|
||||
---
|
||||
|
||||
@@ -463,20 +399,6 @@ Legacy removal starts only when all of the following are true:
|
||||
- Bridge instrumentation shows zero fallback-path usage in integration and e2e
|
||||
suites.
|
||||
- A rollback plan exists for each removal PR until the release is cut.
|
||||
- ECS write path has run as default behind a kill switch for at least one full
|
||||
release cycle.
|
||||
- No unresolved P0/P1 extension regressions are attributed to ECS migration in
|
||||
that cycle.
|
||||
|
||||
### Phase 5 trigger packet (required before first legacy-removal PR)
|
||||
|
||||
The team prepares a single go/no-go packet containing:
|
||||
|
||||
- Phase 4 -> 5 criteria checklist with links to evidence.
|
||||
- Extension compatibility matrix results.
|
||||
- Bridge fallback usage report (must be zero for the target concern).
|
||||
- Performance gate report for ECS render/read paths.
|
||||
- Rollback owner, rollback steps, and release coordination sign-off.
|
||||
|
||||
---
|
||||
|
||||
@@ -529,93 +451,6 @@ event listeners instead of callbacks.
|
||||
- Callback order remains: output validation -> input validation -> commit ->
|
||||
output change notification -> input change notification.
|
||||
|
||||
### Extension Migration Examples (old -> new)
|
||||
|
||||
The bridge keeps legacy callbacks working, but extension authors can migrate
|
||||
incrementally to ECS-native patterns.
|
||||
|
||||
#### 1) Widget lookup by name
|
||||
|
||||
```ts
|
||||
// Legacy pattern
|
||||
const seedWidget = node.widgets?.find((w) => w.name === 'seed')
|
||||
seedWidget?.setValue(42)
|
||||
|
||||
// ECS pattern (using the bridge/world widget lookup index)
|
||||
const seedWidgetId = world.widgetIndex.getByNodeAndName(nodeId, 'seed')
|
||||
if (seedWidgetId) {
|
||||
const widgetValue = world.getComponent(seedWidgetId, WidgetValue)
|
||||
if (widgetValue) {
|
||||
world.setComponent(seedWidgetId, WidgetValue, {
|
||||
...widgetValue,
|
||||
value: 42
|
||||
})
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
#### 2) `onConnectionsChange` callback
|
||||
|
||||
```ts
|
||||
// Legacy pattern
|
||||
nodeType.prototype.onConnectionsChange = function (
|
||||
side,
|
||||
slot,
|
||||
connected,
|
||||
linkInfo
|
||||
) {
|
||||
updateExtensionState(this.id, side, slot, connected, linkInfo)
|
||||
}
|
||||
|
||||
// ECS pattern
|
||||
lifecycleEvents.on('connection.changed', (event) => {
|
||||
if (event.nodeId !== nodeId) return
|
||||
updateExtensionState(
|
||||
event.nodeId,
|
||||
event.side,
|
||||
event.slotIndex,
|
||||
event.connected,
|
||||
event.linkInfo
|
||||
)
|
||||
})
|
||||
```
|
||||
|
||||
#### 3) `onRemoved` callback
|
||||
|
||||
```ts
|
||||
// Legacy pattern
|
||||
nodeType.prototype.onRemoved = function () {
|
||||
cleanupExtensionResources(this.id)
|
||||
}
|
||||
|
||||
// ECS pattern
|
||||
lifecycleEvents.on('entity.removed', (event) => {
|
||||
if (event.kind !== 'node' || event.entityId !== nodeId) return
|
||||
cleanupExtensionResources(event.entityId)
|
||||
})
|
||||
```
|
||||
|
||||
#### 4) `graph._version++`
|
||||
|
||||
```ts
|
||||
// Legacy pattern (do not add new usages)
|
||||
graph._version++
|
||||
|
||||
// Bridge-safe transitional pattern (Phase 0a)
|
||||
graph.incrementVersion()
|
||||
|
||||
// ECS-native pattern: mutate through command/system API.
|
||||
// VersionSystem bumps once at transaction commit.
|
||||
executor.run({
|
||||
type: 'SetWidgetValue',
|
||||
execute(world) {
|
||||
const value = world.getComponent(widgetId, WidgetValue)
|
||||
if (!value) return
|
||||
world.setComponent(widgetId, WidgetValue, { ...value, value: 42 })
|
||||
}
|
||||
})
|
||||
```
|
||||
|
||||
**Question to resolve after compatibility parity:**
|
||||
|
||||
- Should ECS-native lifecycle events stay synchronous after bridge removal, or
|
||||
@@ -636,9 +471,6 @@ state between these calls.
|
||||
- Mutating systems run inside `world.transaction(label, fn)`.
|
||||
- The bridge maps one World transaction to one `beforeChange()` /
|
||||
`afterChange()` bracket.
|
||||
- Operations with multiple component writes (for example `connect()` touching
|
||||
slots, links, and node metadata) still commit as one transaction and therefore
|
||||
one undo entry.
|
||||
- Failed transactions do not publish partial writes, lifecycle events, or
|
||||
version increments.
|
||||
|
||||
@@ -699,7 +531,6 @@ Phase 3->4 gate checklist ──────── depends on 3a, 3b, 3c
|
||||
Phase 4a (Position writes) ────── depends on 2a, 3b
|
||||
Phase 4b (Connectivity mutations) ─ depends on 3c, 3->4 gate
|
||||
Phase 4c (Widget writes) ─────── depends on 2b
|
||||
Phase 4d (Layout decoupling) ─── depends on 2a, 3->4 gate
|
||||
|
||||
Phase 4->5 exit criteria ──────── depends on all of Phase 4
|
||||
|
||||
|
||||
Reference in New Issue
Block a user