diff --git a/.jules/bolt.md b/.jules/bolt.md index 4a0a090..638a589 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -9,3 +9,7 @@ ## 2024-05-23 - Read-Write-Read Layout Thrashing **Learning:** Writing to the DOM (e.g., `element.style.top = ...`) immediately invalidates the layout. If you subsequently read a layout property (e.g., `getBoundingClientRect()`) in the same frame, the browser must force a synchronous layout recalculation. **Action:** In event handlers that move elements, Read all necessary dimensions first, then Perform all Writes. If a downstream method (like `updateMagnifiedView`) needs dimensions, pass the cached values instead of re-reading them. + +## 2026-02-09 - Redundant Node Iterations +**Learning:** Iterating `app.graph._nodes` multiple times per frame (e.g., once for text, once for titles, once for images) is a significant bottleneck in extensions, especially with large graphs. Each traversal is O(N). +**Action:** Calculate the visible subset of nodes ONCE per frame (`getVisibleNodes`) and reuse this list for all subsequent rendering passes. diff --git a/src/magnify-glass/OffscreenRenderer.ts b/src/magnify-glass/OffscreenRenderer.ts index 0f4e4c3..b61dee4 100644 --- a/src/magnify-glass/OffscreenRenderer.ts +++ b/src/magnify-glass/OffscreenRenderer.ts @@ -88,6 +88,41 @@ export class OffscreenRenderer { } } + /** + * Get a list of nodes visible within the source rectangle. + * This avoids iterating over all nodes multiple times per frame. + */ + private getVisibleNodes( + sourceCssX: number, + sourceCssY: number, + sourceCssWidth: number, + sourceCssHeight: number, + scale: number, + offset: [number, number] + ): any[] { + const graph = app?.graph; + if (!graph || !graph._nodes) return []; + + const visibleNodes = []; + for (const node of graph._nodes) { + if (!node.pos || !node.size) continue; + if (node.flags?.collapsed) continue; + + // Calculate node position in CSS pixels + const nodeCssX = node.pos[0] * scale + offset[0]; + const nodeCssY = node.pos[1] * scale + offset[1]; + const nodeCssWidth = node.size[0] * scale; + const nodeCssHeight = node.size[1] * scale; + + // Check overlap + if (nodeCssX + nodeCssWidth < sourceCssX || nodeCssX > sourceCssX + sourceCssWidth) continue; + if (nodeCssY + nodeCssHeight < sourceCssY || nodeCssY > sourceCssY + sourceCssHeight) continue; + + visibleNodes.push(node); + } + return visibleNodes; + } + /** * Detect if there are nodes with image previews in the capture region. * These nodes (Save Image, Preview Image, etc.) have images that are drawn @@ -191,11 +226,19 @@ export class OffscreenRenderer { const lgCanvas = app?.canvas; const currentScale = lgCanvas?.ds?.scale ?? 1.0; const currentOffset: [number, number] = lgCanvas?.ds?.offset ? [lgCanvas.ds.offset[0], lgCanvas.ds.offset[1]] : [0, 0]; - this.drawWidgetTextNatively(sourceX, sourceY, sourceWidth, sourceHeight, renderSize, currentScale, currentOffset); + + // OPTIMIZATION: Get visible nodes once + const sourceCssX = sourceX / dpr; + const sourceCssY = sourceY / dpr; + const sourceCssWidth = sourceWidth / dpr; + const sourceCssHeight = sourceHeight / dpr; + const visibleNodes = this.getVisibleNodes(sourceCssX, sourceCssY, sourceCssWidth, sourceCssHeight, currentScale, currentOffset); + + this.drawWidgetTextNatively(visibleNodes, sourceX, sourceY, sourceWidth, sourceHeight, renderSize, currentScale, currentOffset); // Draw node titles if emphasis is enabled if (this.config.accessibilityEnabled && this.config.nodeTitleEmphasis) { - this.drawNodeTitlesNatively(sourceX, sourceY, sourceWidth, sourceHeight, renderSize, currentScale, currentOffset); + this.drawNodeTitlesNatively(visibleNodes, sourceX, sourceY, sourceWidth, sourceHeight, renderSize, currentScale, currentOffset); } // NOTE: We do NOT call drawImagePreviewsNatively() here. @@ -346,15 +389,23 @@ export class OffscreenRenderer { // Note: For virtual zoom, we use targetScale and the CURRENT offset (after setZoom) // because lgCanvas.setZoom() modified the offset to zoom around the pivot const captureOffset: [number, number] = [lgCanvas.ds.offset[0], lgCanvas.ds.offset[1]]; - this.drawWidgetTextNatively(sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); + + // OPTIMIZATION: Get visible nodes once + const sourceCssX = sourceX / dpr; + const sourceCssY = sourceY / dpr; + const sourceCssWidth = sourceWidth / dpr; + const sourceCssHeight = sourceHeight / dpr; + const visibleNodes = this.getVisibleNodes(sourceCssX, sourceCssY, sourceCssWidth, sourceCssHeight, targetScale, captureOffset); + + this.drawWidgetTextNatively(visibleNodes, sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); // Draw node titles if emphasis is enabled if (this.config.accessibilityEnabled && this.config.nodeTitleEmphasis) { - this.drawNodeTitlesNatively(sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); + this.drawNodeTitlesNatively(visibleNodes, sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); } // Draw image/video previews natively on top of the captured canvas - this.drawImagePreviewsNatively(sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); + this.drawImagePreviewsNatively(visibleNodes, sourceX, sourceY, sourceWidth, sourceHeight, renderSize, targetScale, captureOffset); // Draw cursor preview overlay if enabled if (this.config.showCursorPreview) { @@ -400,6 +451,7 @@ export class OffscreenRenderer { * Draw node titles natively with accessibility styling. */ private drawNodeTitlesNatively( + visibleNodes: any[], sourceX: number, sourceY: number, sourceWidth: number, @@ -408,8 +460,7 @@ export class OffscreenRenderer { scale: number, offset: [number, number] ): void { - const graph = app?.graph; - if (!graph || !graph._nodes || !this.offscreenCtx) return; + if (!this.offscreenCtx) return; const ctx = this.offscreenCtx; const TITLE_HEIGHT = 30; @@ -425,7 +476,7 @@ export class OffscreenRenderer { const sourceCssWidth = sourceWidth / actualDpr; const sourceCssHeight = sourceHeight / actualDpr; - for (const node of graph._nodes) { + for (const node of visibleNodes) { if (!node.pos || !node.size) continue; if (node.flags?.collapsed) continue; @@ -497,6 +548,7 @@ export class OffscreenRenderer { * Draw widget text natively on the offscreen canvas. * This renders text content that would otherwise be lost since widgets are DOM elements. * + * @param visibleNodes - List of nodes to check for text widgets * @param sourceX - Source X position in backing pixels * @param sourceY - Source Y position in backing pixels * @param sourceWidth - Source width in backing pixels @@ -506,6 +558,7 @@ export class OffscreenRenderer { * @param offset - Canvas offset [x, y] used during capture */ private drawWidgetTextNatively( + visibleNodes: any[], sourceX: number, sourceY: number, sourceWidth: number, @@ -514,8 +567,7 @@ export class OffscreenRenderer { scale: number, offset: [number, number] ): void { - const graph = app?.graph; - if (!graph || !graph._nodes || !this.offscreenCtx) return; + if (!this.offscreenCtx) return; const ctx = this.offscreenCtx; const lgCanvas = app?.canvas; @@ -546,7 +598,7 @@ export class OffscreenRenderer { const sourceCssWidth = sourceWidth / actualDpr; const sourceCssHeight = sourceHeight / actualDpr; - for (const node of graph._nodes) { + for (const node of visibleNodes) { if (!node.widgets || !node.pos || !node.size) continue; if (node.flags?.collapsed) continue; @@ -711,6 +763,7 @@ export class OffscreenRenderer { * - node.imgs[] array (standard ComfyUI SaveImage/PreviewImage nodes) * - VHS-style DOM widget previews (video/image elements) * + * @param visibleNodes - List of nodes to check for image previews * @param sourceX - Source X position in backing pixels * @param sourceY - Source Y position in backing pixels * @param sourceWidth - Source width in backing pixels @@ -720,6 +773,7 @@ export class OffscreenRenderer { * @param offset - Canvas offset [x, y] used during capture */ private drawImagePreviewsNatively( + visibleNodes: any[], sourceX: number, sourceY: number, sourceWidth: number, @@ -728,8 +782,7 @@ export class OffscreenRenderer { scale: number, offset: [number, number] ): void { - const graph = app?.graph; - if (!graph || !graph._nodes || !this.offscreenCtx) return; + if (!this.offscreenCtx) return; const ctx = this.offscreenCtx; @@ -748,7 +801,7 @@ export class OffscreenRenderer { const TITLE_HEIGHT = 30; const WIDGET_MARGIN = 4; - for (const node of graph._nodes) { + for (const node of visibleNodes) { if (!node.pos || !node.size) continue; if (node.flags?.collapsed) continue; diff --git a/tests/unit/OffscreenRenderer.test.ts b/tests/unit/OffscreenRenderer.test.ts new file mode 100644 index 0000000..89cf8d2 --- /dev/null +++ b/tests/unit/OffscreenRenderer.test.ts @@ -0,0 +1,158 @@ + +import { describe, it, expect, beforeEach, vi, afterEach } from 'vitest'; + +// Create hoisted mocks +const { appMock } = vi.hoisted(() => { + return { + appMock: { + graph: { + _nodes: [] as any[] + }, + canvas: { + ds: { + scale: 1, + offset: [0, 0] + } + } + } + }; +}); + +// Attach to global +// @ts-ignore +globalThis.app = appMock; + +vi.mock('/scripts/app.js', () => ({ + app: appMock +})); + +import { OffscreenRenderer } from '../../src/magnify-glass/OffscreenRenderer'; +import { ConfigManager } from '../../src/magnify-glass/ConfigManager'; +import { MagnifierState } from '../../src/magnify-glass/MagnifierState'; + +describe('OffscreenRenderer Optimization', () => { + let renderer: OffscreenRenderer; + let configMock: ConfigManager; + let stateMock: MagnifierState; + let canvasMock: HTMLCanvasElement; + + beforeEach(() => { + vi.clearAllMocks(); + appMock.graph._nodes = []; + appMock.canvas.ds = { scale: 1, offset: [0, 0] }; + + // Mock ConfigManager + configMock = { + glassSize: 200, + zoomFactor: 2, + accessibilityEnabled: true, + nodeTitleEmphasis: true, + invertColors: false, + grayscaleMode: false, + forceDirectCapture: false, + fontScaleFactor: 100, + boldTextEnabled: false, + textGlowEnabled: false, + textOutlineEnabled: false, + highContrastMode: false, + showCursorPreview: false + } as unknown as ConfigManager; + + // Mock MagnifierState + stateMock = { + x: 100, // Cursor backing X + y: 100, // Cursor backing Y + canvasScale: 1 + } as unknown as MagnifierState; + + // Mock canvas creation to return a mock context + const originalCreateElement = document.createElement.bind(document); + vi.spyOn(document, 'createElement').mockImplementation((tagName) => { + if (tagName === 'canvas') { + const canvas = originalCreateElement(tagName) as HTMLCanvasElement; + canvas.getContext = vi.fn().mockReturnValue({ + clearRect: vi.fn(), + save: vi.fn(), + restore: vi.fn(), + drawImage: vi.fn(), + beginPath: vi.fn(), + roundRect: vi.fn(), + fill: vi.fn(), + stroke: vi.fn(), + fillText: vi.fn(), + measureText: vi.fn().mockReturnValue({ width: 10 }), + clip: vi.fn(), + scale: vi.fn(), + translate: vi.fn(), + moveTo: vi.fn(), + lineTo: vi.fn(), + closePath: vi.fn(), + strokeText: vi.fn(), + }); + return canvas; + } + return originalCreateElement(tagName); + }); + + renderer = new OffscreenRenderer(configMock, stateMock); + }); + + it('should identify visible nodes and render them', () => { + // Setup nodes + // Node 1: Visible (at 50, 50, size 100x100) + // Cursor at 100, 100. Glass size 200, zoom 2. Source size 100. + // Capture region: 50 to 150. + // Node 1 overlaps. + const visibleNode = { + pos: [50, 50], // Graph coords + size: [100, 100], + widgets: [ + { type: 'text', name: 'visible_widget', value: 'Visible Text', computedHeight: 50, last_y: 40 } + ] + }; + // Node 2: Not Visible (far away) + const invisibleNode = { + pos: [1000, 1000], + size: [100, 100], + widgets: [ + { type: 'text', name: 'invisible_widget', value: 'Invisible Text', computedHeight: 50, last_y: 40 } + ] + }; + + appMock.graph._nodes = [visibleNode, invisibleNode]; + + // Mock target canvas + canvasMock = document.createElement('canvas'); + canvasMock.width = 1000; + canvasMock.height = 800; + vi.spyOn(canvasMock, 'getBoundingClientRect').mockReturnValue({ + x: 0, y: 0, width: 1000, height: 800, top: 0, left: 0, right: 1000, bottom: 800, toJSON: () => {} + } as DOMRect); + + // Spy on private method drawWidgetTextNatively via cast + const drawSpy = vi.spyOn(renderer as any, 'drawWidgetTextNatively'); + + // Spy on context fillText to verify rendering + // Need to access the internal context. + const ctx = (renderer as any).offscreenCtx; + const fillTextSpy = vi.spyOn(ctx, 'fillText'); + + // Render + renderer.renderHighResRegion(canvasMock); + + // Verify drawWidgetTextNatively was called + expect(drawSpy).toHaveBeenCalled(); + + // Verify that visibleNodes list was correctly filtered and passed + const callArgs = drawSpy.mock.calls[0]; + const visibleNodes = callArgs[0] as any[]; + expect(visibleNodes.length).toBe(1); + expect(visibleNodes[0].widgets[0].value).toBe('Visible Text'); + + // Verify text was drawn for visible node + expect(fillTextSpy).toHaveBeenCalledWith(expect.stringContaining('Visible Text'), expect.any(Number), expect.any(Number)); + + // Verify text was NOT drawn for invisible node + expect(fillTextSpy).not.toHaveBeenCalledWith(expect.stringContaining('Invisible Text'), expect.any(Number), expect.any(Number)); + }); +});