mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-01-26 10:59:53 +00:00
Speed: Remove some optimizations that weren't optimizing (#6209)
## Summary Simplify the TransformPane. ## Changes - **What**: Remove the settling and culling composables. Gets rid of a frequent event emission and some event handling addition/removals. ## Review Focus In testing with a huge workflow in Vue mode, it was a lot faster without these than with. Can you check to see if you experience the same benefits? ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6209-Speed-Remove-some-optimizations-that-weren-t-optimizing-2946d73d3650815197a4df3c58a61575) by [Unito](https://www.unito.io) --------- Co-authored-by: github-actions <github-actions@github.com>
This commit is contained in:
Binary file not shown.
|
Before Width: | Height: | Size: 36 KiB After Width: | Height: | Size: 38 KiB |
@@ -1315,15 +1315,6 @@ audio.comfy-audio.empty-audio-widget {
|
||||
font-size 0.1s ease;
|
||||
}
|
||||
|
||||
/* Performance optimization during canvas interaction */
|
||||
.transform-pane--interacting .lg-node * {
|
||||
transition: none !important;
|
||||
}
|
||||
|
||||
.transform-pane--interacting .lg-node {
|
||||
will-change: transform;
|
||||
}
|
||||
|
||||
/* ===================== Mask Editor Styles ===================== */
|
||||
/* To be migrated to Tailwind later */
|
||||
#maskEditor_brush {
|
||||
|
||||
@@ -57,7 +57,6 @@
|
||||
<TransformPane
|
||||
v-if="shouldRenderVueNodes && comfyApp.canvas && comfyAppReady"
|
||||
:canvas="comfyApp.canvas"
|
||||
@transform-update="handleTransformUpdate"
|
||||
@wheel.capture="canvasInteractions.forwardEventToCanvas"
|
||||
>
|
||||
<!-- Vue nodes rendered based on graph nodes -->
|
||||
@@ -118,7 +117,6 @@ import TopbarBadges from '@/components/topbar/TopbarBadges.vue'
|
||||
import WorkflowTabs from '@/components/topbar/WorkflowTabs.vue'
|
||||
import { useChainCallback } from '@/composables/functional/useChainCallback'
|
||||
import type { VueNodeData } from '@/composables/graph/useGraphNodeManager'
|
||||
import { useViewportCulling } from '@/composables/graph/useViewportCulling'
|
||||
import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle'
|
||||
import { useNodeBadge } from '@/composables/node/useNodeBadge'
|
||||
import { useCanvasDrop } from '@/composables/useCanvasDrop'
|
||||
@@ -202,7 +200,6 @@ const { shouldRenderVueNodes } = useVueFeatureFlags()
|
||||
|
||||
// Vue node system
|
||||
const vueNodeLifecycle = useVueNodeLifecycle()
|
||||
const { handleTransformUpdate } = useViewportCulling()
|
||||
|
||||
const handleVueNodeLifecycleReset = async () => {
|
||||
if (shouldRenderVueNodes.value) {
|
||||
|
||||
@@ -1,103 +0,0 @@
|
||||
/**
|
||||
* Vue Nodes Viewport Culling
|
||||
*
|
||||
* Principles:
|
||||
* 1. Query DOM directly using data attributes (no cache to maintain)
|
||||
* 2. Set display none on element to avoid cascade resolution overhead
|
||||
* 3. Only run when transform changes (event driven)
|
||||
*/
|
||||
import { createSharedComposable, useThrottleFn } from '@vueuse/core'
|
||||
import { computed } from 'vue'
|
||||
|
||||
import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle'
|
||||
import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { app } from '@/scripts/app'
|
||||
|
||||
type Bounds = [left: number, right: number, top: number, bottom: number]
|
||||
|
||||
function getNodeBounds(node: LGraphNode): Bounds {
|
||||
const [nodeLeft, nodeTop] = node.pos
|
||||
const nodeRight = nodeLeft + node.size[0]
|
||||
const nodeBottom = nodeTop + node.size[1]
|
||||
return [nodeLeft, nodeRight, nodeTop, nodeBottom]
|
||||
}
|
||||
|
||||
function viewportEdges(
|
||||
canvas: ReturnType<typeof useCanvasStore>['canvas']
|
||||
): Bounds | undefined {
|
||||
if (!canvas) {
|
||||
return
|
||||
}
|
||||
const ds = canvas.ds
|
||||
const viewport_width = canvas.canvas.width
|
||||
const viewport_height = canvas.canvas.height
|
||||
const margin = 500 * ds.scale
|
||||
|
||||
const [xOffset, yOffset] = ds.offset
|
||||
|
||||
const leftEdge = -margin / ds.scale - xOffset
|
||||
const rightEdge = (viewport_width + margin) / ds.scale - xOffset
|
||||
const topEdge = -margin / ds.scale - yOffset
|
||||
const bottomEdge = (viewport_height + margin) / ds.scale - yOffset
|
||||
return [leftEdge, rightEdge, topEdge, bottomEdge]
|
||||
}
|
||||
|
||||
function boundsIntersect(boxA: Bounds, boxB: Bounds): boolean {
|
||||
const [aLeft, aRight, aTop, aBottom] = boxA
|
||||
const [bLeft, bRight, bTop, bBottom] = boxB
|
||||
|
||||
const leftOf = aRight < bLeft
|
||||
const rightOf = aLeft > bRight
|
||||
const above = aBottom < bTop
|
||||
const below = aTop > bBottom
|
||||
return !(leftOf || rightOf || above || below)
|
||||
}
|
||||
|
||||
function useViewportCullingIndividual() {
|
||||
const canvasStore = useCanvasStore()
|
||||
const { nodeManager } = useVueNodeLifecycle()
|
||||
|
||||
const viewport = computed(() => viewportEdges(canvasStore.canvas))
|
||||
|
||||
function inViewport(node: LGraphNode | undefined): boolean {
|
||||
if (!viewport.value || !node) {
|
||||
return true
|
||||
}
|
||||
const nodeBounds = getNodeBounds(node)
|
||||
return boundsIntersect(nodeBounds, viewport.value)
|
||||
}
|
||||
|
||||
/**
|
||||
* Update visibility of all nodes based on viewport
|
||||
* Queries DOM directly - no cache maintenance needed
|
||||
*/
|
||||
function updateVisibility() {
|
||||
if (!nodeManager.value || !app.canvas) return // load bearing app.canvas check for workflows being loaded.
|
||||
|
||||
const nodeElements = document.querySelectorAll('[data-node-id]')
|
||||
for (const element of nodeElements) {
|
||||
const nodeId = element.getAttribute('data-node-id')
|
||||
if (!nodeId) continue
|
||||
|
||||
const node = nodeManager.value.getNode(nodeId)
|
||||
if (!node) continue
|
||||
|
||||
const displayValue = inViewport(node) ? '' : 'none'
|
||||
if (
|
||||
element instanceof HTMLElement &&
|
||||
element.style.display !== displayValue
|
||||
) {
|
||||
element.style.display = displayValue
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
const handleTransformUpdate = useThrottleFn(() => updateVisibility, 100, true)
|
||||
|
||||
return { handleTransformUpdate }
|
||||
}
|
||||
|
||||
export const useViewportCulling = createSharedComposable(
|
||||
useViewportCullingIndividual
|
||||
)
|
||||
@@ -115,89 +115,6 @@ describe('TransformPane', () => {
|
||||
const transformState = useTransformState()
|
||||
expect(transformState.syncWithCanvas).toHaveBeenCalledWith(mockCanvas)
|
||||
})
|
||||
|
||||
it('should emit transform update timing', async () => {
|
||||
const mockCanvas = createMockCanvas()
|
||||
const wrapper = mount(TransformPane, {
|
||||
props: {
|
||||
canvas: mockCanvas
|
||||
}
|
||||
})
|
||||
|
||||
await nextTick()
|
||||
|
||||
// Allow RAF to execute
|
||||
vi.advanceTimersToNextFrame()
|
||||
|
||||
expect(wrapper.emitted('transformUpdate')).toBeTruthy()
|
||||
})
|
||||
})
|
||||
|
||||
describe('canvas event listeners', () => {
|
||||
it('should add event listeners to canvas on mount', async () => {
|
||||
const mockCanvas = createMockCanvas()
|
||||
mount(TransformPane, {
|
||||
props: {
|
||||
canvas: mockCanvas
|
||||
}
|
||||
})
|
||||
|
||||
await nextTick()
|
||||
|
||||
expect(mockCanvas.canvas.addEventListener).toHaveBeenCalledWith(
|
||||
'wheel',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
|
||||
'pointerdown',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
|
||||
'pointerup',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.addEventListener).not.toHaveBeenCalledWith(
|
||||
'pointercancel',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
})
|
||||
|
||||
it('should remove event listeners on unmount', async () => {
|
||||
const mockCanvas = createMockCanvas()
|
||||
const wrapper = mount(TransformPane, {
|
||||
props: {
|
||||
canvas: mockCanvas
|
||||
}
|
||||
})
|
||||
|
||||
await nextTick()
|
||||
wrapper.unmount()
|
||||
|
||||
expect(mockCanvas.canvas.removeEventListener).toHaveBeenCalledWith(
|
||||
'wheel',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
|
||||
'pointerdown',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
|
||||
'pointerup',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
expect(mockCanvas.canvas.removeEventListener).not.toHaveBeenCalledWith(
|
||||
'pointercancel',
|
||||
expect.any(Function),
|
||||
expect.any(Object)
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('interaction state management', () => {
|
||||
@@ -214,9 +131,7 @@ describe('TransformPane', () => {
|
||||
const transformPane = wrapper.find('[data-testid="transform-pane"]')
|
||||
|
||||
// Initially should not have interacting class
|
||||
expect(transformPane.classes()).not.toContain(
|
||||
'transform-pane--interacting'
|
||||
)
|
||||
expect(transformPane.classes()).not.toContain('will-change-transform')
|
||||
})
|
||||
|
||||
it('should handle pointer events for node delegation', async () => {
|
||||
|
||||
@@ -1,12 +1,7 @@
|
||||
<template>
|
||||
<div
|
||||
data-testid="transform-pane"
|
||||
:class="
|
||||
cn(
|
||||
'absolute inset-0 w-full h-full pointer-events-none',
|
||||
isInteracting ? 'transform-pane--interacting' : 'will-change-auto'
|
||||
)
|
||||
"
|
||||
class="absolute inset-0 w-full h-full pointer-events-none"
|
||||
:style="transformStyle"
|
||||
>
|
||||
<!-- Vue nodes will be rendered here -->
|
||||
@@ -16,12 +11,9 @@
|
||||
|
||||
<script setup lang="ts">
|
||||
import { useRafFn } from '@vueuse/core'
|
||||
import { computed } from 'vue'
|
||||
|
||||
import type { LGraphCanvas } from '@/lib/litegraph/src/litegraph'
|
||||
import { useTransformSettling } from '@/renderer/core/layout/transform/useTransformSettling'
|
||||
import { useTransformState } from '@/renderer/core/layout/transform/useTransformState'
|
||||
import { cn } from '@/utils/tailwindUtil'
|
||||
|
||||
interface TransformPaneProps {
|
||||
canvas?: LGraphCanvas
|
||||
@@ -31,29 +23,13 @@ const props = defineProps<TransformPaneProps>()
|
||||
|
||||
const { transformStyle, syncWithCanvas } = useTransformState()
|
||||
|
||||
const canvasElement = computed(() => props.canvas?.canvas)
|
||||
const { isTransforming: isInteracting } = useTransformSettling(canvasElement, {
|
||||
settleDelay: 512
|
||||
})
|
||||
|
||||
const emit = defineEmits<{
|
||||
transformUpdate: []
|
||||
}>()
|
||||
|
||||
useRafFn(
|
||||
() => {
|
||||
if (!props.canvas) {
|
||||
return
|
||||
}
|
||||
syncWithCanvas(props.canvas)
|
||||
emit('transformUpdate')
|
||||
},
|
||||
{ immediate: true }
|
||||
)
|
||||
</script>
|
||||
|
||||
<style scoped>
|
||||
.transform-pane--interacting {
|
||||
will-change: transform;
|
||||
}
|
||||
</style>
|
||||
|
||||
@@ -1,84 +0,0 @@
|
||||
import { useDebounceFn, useEventListener } from '@vueuse/core'
|
||||
import { ref } from 'vue'
|
||||
import type { MaybeRefOrGetter } from 'vue'
|
||||
|
||||
interface TransformSettlingOptions {
|
||||
/**
|
||||
* Delay in ms before transform is considered "settled" after last interaction
|
||||
* @default 200
|
||||
*/
|
||||
settleDelay?: number
|
||||
/**
|
||||
* Whether to use passive event listeners (better performance but can't preventDefault)
|
||||
* @default true
|
||||
*/
|
||||
passive?: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Tracks when canvas zoom transforms are actively changing vs settled.
|
||||
*
|
||||
* This composable helps optimize rendering quality during zoom transformations.
|
||||
* When the user is actively zooming, we can reduce rendering quality
|
||||
* for better performance. Once the transform "settles" (stops changing), we can
|
||||
* trigger high-quality re-rasterization.
|
||||
*
|
||||
* The settling concept prevents constant quality switching during interactions
|
||||
* by waiting for a period of inactivity before considering the transform complete.
|
||||
*
|
||||
* Uses VueUse's useEventListener for automatic cleanup and useDebounceFn for
|
||||
* efficient settle detection.
|
||||
*
|
||||
* @example
|
||||
* ```ts
|
||||
* const { isTransforming } = useTransformSettling(canvasRef, {
|
||||
* settleDelay: 200
|
||||
* })
|
||||
*
|
||||
* // Use in CSS classes or rendering logic
|
||||
* const cssClass = computed(() => ({
|
||||
* 'low-quality': isTransforming.value,
|
||||
* 'high-quality': !isTransforming.value
|
||||
* }))
|
||||
* ```
|
||||
*/
|
||||
export function useTransformSettling(
|
||||
target: MaybeRefOrGetter<HTMLElement | null | undefined>,
|
||||
options: TransformSettlingOptions = {}
|
||||
) {
|
||||
const { settleDelay = 200, passive = true } = options
|
||||
|
||||
const isTransforming = ref(false)
|
||||
|
||||
/**
|
||||
* Mark transform as active
|
||||
*/
|
||||
const markTransformActive = () => {
|
||||
isTransforming.value = true
|
||||
}
|
||||
|
||||
/**
|
||||
* Mark transform as settled (debounced)
|
||||
*/
|
||||
const markTransformSettled = useDebounceFn(() => {
|
||||
isTransforming.value = false
|
||||
}, settleDelay)
|
||||
|
||||
/**
|
||||
* Handle zoom transform event - mark active then queue settle
|
||||
*/
|
||||
const handleWheel = () => {
|
||||
markTransformActive()
|
||||
void markTransformSettled()
|
||||
}
|
||||
|
||||
// Register wheel event listener with auto-cleanup
|
||||
useEventListener(target, 'wheel', handleWheel, {
|
||||
capture: true,
|
||||
passive
|
||||
})
|
||||
|
||||
return {
|
||||
isTransforming
|
||||
}
|
||||
}
|
||||
@@ -1,167 +0,0 @@
|
||||
import { mount } from '@vue/test-utils'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { nextTick, ref } from 'vue'
|
||||
|
||||
import { useTransformSettling } from '@/renderer/core/layout/transform/useTransformSettling'
|
||||
|
||||
describe('useTransformSettling', () => {
|
||||
let element: HTMLDivElement
|
||||
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
element = document.createElement('div')
|
||||
document.body.appendChild(element)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
document.body.removeChild(element)
|
||||
})
|
||||
|
||||
it('should track wheel events and settle after delay', async () => {
|
||||
const { isTransforming } = useTransformSettling(element)
|
||||
|
||||
// Initially not transforming
|
||||
expect(isTransforming.value).toBe(false)
|
||||
|
||||
// Dispatch wheel event
|
||||
element.dispatchEvent(new WheelEvent('wheel', { bubbles: true }))
|
||||
await nextTick()
|
||||
|
||||
// Should be transforming
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
// Advance time but not past settle delay
|
||||
vi.advanceTimersByTime(100)
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
// Advance past settle delay (default 200ms)
|
||||
vi.advanceTimersByTime(150)
|
||||
expect(isTransforming.value).toBe(false)
|
||||
})
|
||||
|
||||
it('should reset settle timer on subsequent wheel events', async () => {
|
||||
const { isTransforming } = useTransformSettling(element, {
|
||||
settleDelay: 300
|
||||
})
|
||||
|
||||
// First wheel event
|
||||
element.dispatchEvent(new WheelEvent('wheel', { bubbles: true }))
|
||||
await nextTick()
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
// Advance time partially
|
||||
vi.advanceTimersByTime(200)
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
// Another wheel event should reset the timer
|
||||
element.dispatchEvent(new WheelEvent('wheel', { bubbles: true }))
|
||||
await nextTick()
|
||||
|
||||
// Advance 200ms more - should still be transforming
|
||||
vi.advanceTimersByTime(200)
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
// Need another 100ms to settle (300ms total from last event)
|
||||
vi.advanceTimersByTime(100)
|
||||
expect(isTransforming.value).toBe(false)
|
||||
})
|
||||
|
||||
it('should not track pan events', async () => {
|
||||
const { isTransforming } = useTransformSettling(element)
|
||||
|
||||
// Pointer events should not trigger transform
|
||||
element.dispatchEvent(new PointerEvent('pointerdown', { bubbles: true }))
|
||||
element.dispatchEvent(new PointerEvent('pointermove', { bubbles: true }))
|
||||
await nextTick()
|
||||
|
||||
expect(isTransforming.value).toBe(false)
|
||||
})
|
||||
|
||||
it('should work with ref target', async () => {
|
||||
const targetRef = ref<HTMLElement | null>(null)
|
||||
const { isTransforming } = useTransformSettling(targetRef)
|
||||
|
||||
// No target yet
|
||||
expect(isTransforming.value).toBe(false)
|
||||
|
||||
// Set target
|
||||
targetRef.value = element
|
||||
await nextTick()
|
||||
|
||||
// Now events should work
|
||||
element.dispatchEvent(new WheelEvent('wheel', { bubbles: true }))
|
||||
await nextTick()
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
vi.advanceTimersByTime(200)
|
||||
expect(isTransforming.value).toBe(false)
|
||||
})
|
||||
|
||||
it('should use capture phase for events', async () => {
|
||||
const captureHandler = vi.fn()
|
||||
const bubbleHandler = vi.fn()
|
||||
|
||||
// Add handlers to verify capture phase
|
||||
element.addEventListener('wheel', captureHandler, true)
|
||||
element.addEventListener('wheel', bubbleHandler, false)
|
||||
|
||||
const { isTransforming } = useTransformSettling(element)
|
||||
|
||||
// Create child element
|
||||
const child = document.createElement('div')
|
||||
element.appendChild(child)
|
||||
|
||||
// Dispatch event on child
|
||||
child.dispatchEvent(new WheelEvent('wheel', { bubbles: true }))
|
||||
await nextTick()
|
||||
|
||||
// Capture handler should be called before bubble handler
|
||||
expect(captureHandler).toHaveBeenCalled()
|
||||
expect(isTransforming.value).toBe(true)
|
||||
|
||||
element.removeEventListener('wheel', captureHandler, true)
|
||||
element.removeEventListener('wheel', bubbleHandler, false)
|
||||
})
|
||||
|
||||
it('should clean up event listeners when component unmounts', async () => {
|
||||
const removeEventListenerSpy = vi.spyOn(element, 'removeEventListener')
|
||||
|
||||
// Create a test component
|
||||
const TestComponent = {
|
||||
setup() {
|
||||
const { isTransforming } = useTransformSettling(element)
|
||||
return { isTransforming }
|
||||
},
|
||||
template: '<div>{{ isTransforming }}</div>'
|
||||
}
|
||||
|
||||
const wrapper = mount(TestComponent)
|
||||
await nextTick()
|
||||
|
||||
// Unmount component
|
||||
wrapper.unmount()
|
||||
|
||||
// Should have removed wheel event listener
|
||||
expect(removeEventListenerSpy).toHaveBeenCalledWith(
|
||||
'wheel',
|
||||
expect.any(Function),
|
||||
expect.objectContaining({ capture: true })
|
||||
)
|
||||
})
|
||||
|
||||
it('should use passive listeners when specified', async () => {
|
||||
const addEventListenerSpy = vi.spyOn(element, 'addEventListener')
|
||||
|
||||
useTransformSettling(element, {
|
||||
passive: true
|
||||
})
|
||||
|
||||
// Check that passive option was used for wheel event
|
||||
expect(addEventListenerSpy).toHaveBeenCalledWith(
|
||||
'wheel',
|
||||
expect.any(Function),
|
||||
expect.objectContaining({ passive: true, capture: true })
|
||||
)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user