Repository navigation
[Component] Feature: Demo sprite hover and ruler highlights - #3763
wenjiangping wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #3763 +/- ##
==========================================
+ Coverage 58.60% 59.87% +1.27%
==========================================
Files 771 773 +2
Lines 27592 27897 +305
Branches 5195 5269 +74
==========================================
+ Hits 16169 16703 +534
+ Misses 9964 9678 -286
- Partials 1459 1516 +57
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
The hover and Ruler highlight wiring looks sound overall. Previews are cleared on leave, typing, document change, blur and unmount, and thumbnails hide the outline. I found no security issues. The inline comments below cover a few performance and documentation points.
One point that can't be placed inline: docs/product/tutorial-v2.zh.md has no counterpart to the new "Sprite highlighting demo (#3754)" subsection. The Chinese doc otherwise mirrors the English one section by section, so please add a translation or note that it will follow.
|
|
||
| // Smooth the mask before expanding it so fine alpha noise does not become a jagged outline. | ||
| // Supersampling keeps both edges antialiased at a fixed screen thickness. | ||
| const outline = computed(() => { |
There was a problem hiding this comment.
This computed allocates a new canvas and calls drawImage 66 times every time it re-runs, and it re-runs more often than it needs to:
- It depends on the whole
spriteConfig, so any change rebuilds it, includingx/y/rotation/visible. - It also rebuilds on every
mapScalechange, for example while a panel is being resized.
Large or SVG costumes at a high stage scale could cause hover jank. Consider:
- Keying the bitmap only on
image,width,height,abs(scaleX)andmapScale. - Caching the result per image and scale, so hovering the same sprite again or moving the Ruler snap between sprites doesn't rebuild it.
| heading: sprite.heading | ||
| })) | ||
| editorCtx.project.sprites | ||
| .filter((sprite) => sprite.visible) |
There was a problem hiding this comment.
This filter changes Ruler behavior in both editor modes: hidden sprites are no longer snap targets. That is probably intended, since an invisible sprite shouldn't attract the snap. However, the product doc says Ruler snapping "keeps its existing availability in both editor modes". Please mention this change in the doc or the PR description.
| clear() | ||
| return | ||
| } | ||
| this.hoveredPosition.value = fromMonacoPosition({ |
There was a problem hiding this comment.
Nit: every Monaco mouseMove assigns a new Position object, even when the cursor stays on the same character. This re-triggers the hoveredResource getter, which scans items and slots. The in-place-input branch (L55) also returns a fresh { uri } object, so resourceHover is emitted again and getResourceModel re-runs on each move. Consider skipping the assignment when the line and column haven't changed, or comparing by uri before emitting.
|
@xgopilot /review |
|
The PR review was submitted successfully as a single non-blocking Review completeI reviewed PR #3763 ([Component] Feature: Demo sprite hover and ruler highlights) across code quality, performance, security, and documentation accuracy using four parallel subagents, then deduplicated and verified the findings before posting. Submitted: one non-blocking PR review → #3763 (review) Inline findings (all in
Body notes: duplicated Dropped as low-value/non-issues: |
There was a problem hiding this comment.
Review: Demo sprite hover and ruler highlights
Thorough, well-structured PR. Lifecycle cleanup (timers, listeners, disposers) is handled carefully across StageViewer, StageRuler, SpriteNameLabel, and ResourceReferenceController, and the new tests (bitmap reuse/eviction, Ruler snap-target reporting, resource-hover resolution) give good coverage of the tricky parts. The docs updates (EN/ZH) accurately mirror the implemented behavior.
Findings are concentrated in SpriteHighlight.vue's outline-bitmap generation. None are blocking; the inline notes are worth considering.
Additional minor notes (no inline anchor):
- Duplicated
120grace-period constant. The hover-leave grace period of120ms is repeated independently inStageViewer.vue(handleSpriteHover),SpriteNameLabel.vue(hideTimer), andresource/index.ts(clearLater). Correct, but a shared named constant would keep the three in sync. maxCachedPixelscomment wording.SpriteHighlight.vue:4calls4 * 1024 * 1024a "16 MiB pixel budget". The number is a ~4M pixel count (which is ~16 MiB only after assuming 4 bytes/pixel). The accounting is correct, but wording like "~4M-pixel budget (≈16 MiB RGBA)" would read less ambiguously.
Nice work overall.
| const ctx = canvas.getContext('2d') | ||
| if (ctx == null) return null | ||
|
|
||
| const mask = document.createElement('canvas') |
There was a problem hiding this comment.
Computed getter mutates a module-level cache as a side effect.
The outline computed getter reads from and mutates the module-level outlineCache (the splice/push LRU promotion on a cache hit, and the push + eviction loop on a cache miss around L89-98). Vue computed getters are expected to be pure, and Vue may evaluate a computed more than once in some edge cases, which makes the LRU ordering non-deterministic and can generate a bitmap that is immediately discarded. The heavy imperative work (canvas allocation, getImageData, pixel loops) also doesn't belong in a getter.
Consider moving the bitmap generation + cache bookkeeping into a watch/watchEffect that writes to a shallowRef, keeping outline pure (the downstream config computed is already nicely pure). This matches the watch(..., onCleanup) pattern used elsewhere (e.g. useRenderableImage) and also makes the retained-canvas lifecycle explicit.
| ctx.globalCompositeOperation = 'destination-out' | ||
| drawSilhouette(1.5) | ||
| const result = { canvas, scale: scale * pixelRatio, padding: padding * pixelRatio } | ||
| if (canvas.width * canvas.height <= maxCachedPixels) { |
There was a problem hiding this comment.
Large sprites skip the cache, so the most expensive bitmaps regenerate on every hover.
The cache only stores entries where canvas.width * canvas.height <= maxCachedPixels (~4M backing pixels). With pixelRatio = 2, a sprite whose scaled size approaches ~1024×1024 already exceeds this, so exactly the largest/most-expensive outlines are never cached — each hover-enter (and each re-render that changes the sprite config identity) pays the full getImageData + two full-buffer alpha loops + 2×33 drawImage pipeline again.
Additionally, because scale = abs(scaleX) * mapScale is part of the cache key, a continuous size edit or panel/window resize while a sprite is highlighted produces a new key every frame — each intermediate value misses the cache, regenerates, and is immediately evicted.
Consider clamping the mask working resolution (cap the canvas and upscale on display) and/or quantizing scale in the key, so the heavy path isn't re-run per frame for large sprites and during resize.
| width * scale * pixelRatio, | ||
| height * scale * pixelRatio | ||
| ) | ||
| const maskData = maskCtx.getImageData(0, 0, mask.width, mask.height) |
There was a problem hiding this comment.
getImageData is unguarded against canvas tainting.
This is safe today — sprite images come from useRenderableImage, which uses same-origin blob: URLs, so drawing them does not taint the canvas and getImageData succeeds. But the safety depends entirely on that invariant. If a future caller ever passes a sprite whose image is loaded cross-origin without crossOrigin='anonymous', this throws a SecurityError from inside a computed, surfacing as an unhandled render error. Since the highlight is purely decorative, a try/catch that returns null (degrade to no outline) would make the component robust to the image source.
Related
Related to #3754 (part of #3403).
Background
Learners need to connect sprite references in code with the corresponding sprites on the stage, especially when several small sprites are present. This Course Simple Mode demo adds temporary sprite highlighting for code hover and Ruler measurement, using a smooth white silhouette outline and the existing sprite name label.
Experience changes
Scope
Validation
git diff --check.