-
Notifications
You must be signed in to change notification settings - Fork 50
Sprite Quick Config in Preview #2567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @Overu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the sprite editing experience by introducing a "Sprite Quick Config" feature, primarily focusing on visibility control. It adds a new interactive button to the sprite transformer in the preview, allows users to easily hide or show sprites via a context menu option, and provides a clear visual cue in the sprite list when a sprite is not visible. These changes aim to streamline sprite management and improve user feedback within the editor. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Review SummaryThis PR introduces a well-designed sprite quick configuration system with good component separation. However, several critical issues need attention before merge: Critical Issues:
Key Improvements Needed:
Detailed feedback provided in inline comments. |
spx-gui/src/components/editor/common/viewer/quick-config/QuickConfigWrapper.vue
Show resolved
Hide resolved
| clearTimeout(timer) | ||
| activeInteractions++ | ||
| } | ||
| function handleInteractionEnd() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Counter can go negative
If handleInteractionEnd is called more times than handleInteractionStart (due to event ordering issues), activeInteractions becomes negative and the timer logic breaks.
Fix:
function handleInteractionEnd() {
activeInteractions = Math.max(0, activeInteractions - 1)
if (activeInteractions === 0) {
flush()
}
}
搞不定了 🤣 等晚点 PR 有新改动的话再测试跑 review 看看吧 |
spx-gui/src/components/editor/common/viewer/quick-config/utils.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,41 @@ | |||
| <script lang="ts" setup> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Duplication: Nearly Identical Components
The SizeConfigPanel components for sprite and widget are nearly identical. The only differences are:
- The prop type (
SpritevsWidget) - The v-radar description text
However, the sprite/widget prop is actually unused in the component logic. This duplication violates DRY principles and makes maintenance harder.
Suggested fix: Consolidate into a single generic component in the common folder that doesn't require entity-specific props:
<!-- common/SizeConfigPanel.vue -->
<script setup lang="ts">
defineProps<{
size: number
}>()
defineEmits<{
'update:size': [{ size: number }]
}>()
</script>
<template>
<ConfigPanel>
<UINumberInput
:model-value="size"
v-radar="{ name: 'Size input', desc: 'Input field for entity size percentage' }"
:min="0"
suffix="%"
@update:model-value="$emit('update:size', { size: $event })"
/>
</ConfigPanel>
</template>The same applies to PositionConfigPanel components.
| <template> | ||
| <ConfigPanel> | ||
| <UINumberInput | ||
| v-radar="{ name: 'Size input', desc: 'Input field for monitor size' }" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent Terminology in v-radar Annotations
The v-radar descriptions use "monitor" instead of "widget":
- Line 25:
'Input field for monitor size' - Similar issues in
PositionConfigPanelfor widgets
This inconsistency can confuse users and accessibility tools. The correct term throughout the codebase is "widget" for these UI elements.
Suggested fix:
v-radar="{ name: 'Size input', desc: 'Input field for widget size percentage' }"| const x = round(e.target.x() - mapSize.width / 2) | ||
| const y = round(mapSize.height / 2 - e.target.y()) | ||
| // After Sprite changes, updateXXX events also need to be triggered | ||
| watch(config, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance: Redundant Event Emissions
This watch on the config computed property triggers updateSprite which emits events whenever config changes. Since config depends on multiple sprite properties (x, y, heading, size, visible), this fires frequently.
The issue: This can cause redundant event emissions because:
- User transforms sprite →
notifyUpdateSpritefires via event handlers - Sprite props update →
configcomputed changes → this watch fires →updateSpriteemits events again
This creates a feedback loop where the same transformation triggers events twice, potentially causing double-processing of updates.
Suggested fix: Add a flag to prevent duplicate emissions or reconsider if this watch is necessary given that direct interactions already emit via @dragmove, @dragend, @transform, @transformend handlers.
| function getVisibleChildrenUnionRect(root: Element) { | ||
| let newRect: DOMRect | null = null | ||
|
|
||
| function isVisible(node: Element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performance: Layout Thrashing from getComputedStyle in Loop
The getVisibleChildrenUnionRect function calls window.getComputedStyle() for every element during DOM traversal. This is called frequently during drag/transform operations (via throttled updateQuickConfigPos).
The issue: getComputedStyle() forces style recalculation and can cause layout thrashing, especially with deep DOM trees. This becomes expensive when the quick config popup contains complex Naive UI components.
Suggested fixes:
- Cache computed styles if checking the same element multiple times
- Use cheaper visibility checks first (e.g.,
offsetWidth > 0 && offsetHeight > 0) - Consider using
IntersectionObserverfor visibility detection if applicable - Move this utility to
/workspace/spx-gui/src/utils/dom.tsfor better organization and potential reuse
spx-gui/src/components/editor/common/viewer/quick-config/sprite/DefaultConfigPanel.vue
Show resolved
Hide resolved
| import type { Project } from '@/models/project' | ||
| import type { Widget } from '@/models/widget' | ||
|
|
||
| export function useConfigModal<T>(source: WatchSource<T>, updator: (newValue: T) => void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JSDoc Documentation
These utility functions lack JSDoc documentation explaining their purpose, parameters, and return values. Given their complexity (generic types, debouncing, context providers), clear documentation would significantly improve maintainability.
Suggested fix:
/**
* Creates a reactive ref synchronized with a watch source and provides an update handler.
* @param source - Watch source that provides initial and updated values
* @param updator - Function called to apply updates
* @returns Tuple of [reactive ref, update function]
*/
export function useConfigModal<T>(source: WatchSource<T>, updator: (newValue: T) => void) {
// ...
}
/**
* Wraps a sprite update handler with history action tracking and optional debouncing.
* @param updator - The actual update function to call on the sprite
* @param provideSpriteContext - Function that provides current sprite and project context
* @param withDebounce - Whether to debounce updates (default: true, 300ms delay)
* @returns Wrapped update handler that can be called with new values
*/
export function wrapperSpriteUpdateHandler<T>(...) {
// ...
}
spx-gui/src/components/editor/preview/stage-viewer/StageViewer.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/common/viewer/quick-config/common/ZorderConfigItem.vue
Show resolved
Hide resolved
spx-gui/src/components/editor/common/viewer/quick-config/sprite/DefaultConfigPanel.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/common/viewer/quick-config/QuickConfigWrapper.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/common/viewer/quick-config/QuickConfigWrapper.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/map-editor/map-viewer/MapViewer.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/preview/stage-viewer/StageViewer.vue
Outdated
Show resolved
Hide resolved
spx-gui/src/components/editor/preview/stage-viewer/StageViewer.vue
Outdated
Show resolved
Hide resolved
nighca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
先看这些..细节太多了
Fixed #2513