Skip to content

Optimize ar preview#17134

Open
ProblemShooter wants to merge 2 commits intoAUTOMATIC1111:masterfrom
ProblemShooter:optimize-ar-preview
Open

Optimize ar preview#17134
ProblemShooter wants to merge 2 commits intoAUTOMATIC1111:masterfrom
ProblemShooter:optimize-ar-preview

Conversation

@ProblemShooter
Copy link
Copy Markdown

Refactored the dimensionChange logic and AR preview handling for better performance and readability.
The code now caches repeated DOM queries like gradioApp() and the AR preview div to avoid unnecessary lookups on every input event.
Type conversions were simplified, and redundant calculations were reduced for efficiency.

The AR preview rendering now uses requestAnimationFrame instead of setTimeout for smoother updates, ensuring better user experience.
Event listeners for width and height inputs are added only once, and input value handling is streamlined for clarity.
Overall, this reduces memory usage and improves responsiveness when updating image dimensions.

Additional optimizations include combining repetitive logic for tab selection, scaling, and centering calculations,
making the code easier to maintain and extend in the future. This commit keeps all existing functionality intact while improving performance and maintainability.

Copy link
Copy Markdown

@alvinttang alvinttang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Bug: gradioApp() called at module load time — crashes before Gradio initializes

File: javascript/aspectRatioOverlay.js, lines 4-10

const gradio = gradioApp(); // cache gradioApp reference

let arPreviewRect = gradio.querySelector('#imageARPreview');
if (!arPreviewRect) {
    arPreviewRect = document.createElement('div');
    arPreviewRect.id = "imageARPreview";
    gradio.appendChild(arPreviewRect);
}

This code runs at module parse time (top-level script execution). If the script is loaded before Gradio has fully initialized the DOM, gradioApp() will return null or a stub element, causing gradio.querySelector(...) to throw a TypeError. The original code called gradioApp() inside dimensionChange() (which runs on user input, well after initialization) and inside onAfterUiUpdate() (which runs after UI is ready) — both safe timing points.

Caching gradioApp() at the top level defeats the lazy-initialization pattern that the rest of the codebase relies on.

Bug: requestAnimationFrame + setTimeout interaction causes stale closure over arFrameTimeout

if (arFrameTimeout) cancelAnimationFrame(arFrameTimeout);
arFrameTimeout = requestAnimationFrame(() => {
    setTimeout(() => {
        arPreviewRect.style.display = 'none';
    }, 2000);
});

The original code used clearTimeout(arFrameTimeout) with setTimeout — both operating on the same timer ID space. The new code stores a requestAnimationFrame ID in arFrameTimeout and cancels it with cancelAnimationFrame. However, once the rAF callback fires (typically within ~16ms), the inner setTimeout is scheduled and cannot be cancelledcancelAnimationFrame on subsequent calls won't cancel the already-fired rAF or its inner timeout.

If dimensionChange fires rapidly (e.g., dragging a slider), you get N setTimeout callbacks queued, each hiding the preview after 2 seconds. The original clearTimeout approach correctly cancelled the previous timeout before setting a new one. The rAF wrapper here adds no benefit (the operation is not animation-related) and breaks the cancellation semantics.

Suggested fix: keep setTimeout / clearTimeout as in the original, or store the inner setTimeout ID separately and clear it.

Nit: zoom.js rename zoom to zoomLevel is a breaking change if extensions read elemData

File: extensions-builtin/canvas-zoom-and-pan/javascript/zoom.js, L289

The property name change from zoom to zoomLevel in elemData[elemId] will break any extension or user script that reads elemData[elemId].zoom. If this data structure is part of a public/semi-public API, this needs a deprecation path or at least mention in release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants