Sprite Quick Config in Preview #2567
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a quick configuration feature for sprites in the preview. Specifically, it adds a menu item to toggle a sprite's visibility and displays an icon for hidden sprites. It also adds a new configuration button to the transformer on the stage, though its functionality seems to be a work in progress. The changes are well-structured. I've left a few comments with suggestions for improvement, mainly concerning code clarity and maintainability. This includes correcting some typos (e.g., configor to configurator), replacing magic numbers with named constants, using a more descriptive name for a history action, and simplifying a conditional expression. These changes should make the code easier to read and maintain.
| const configorTagImg = new Image() | ||
| configorTagImg.src = rotatorCirclePng |
There was a problem hiding this comment.
There seems to be a typo in configorTagImg; it should likely be configuratorTagImg. This typo also appears in ConfigorButton and configorButton. It would be good to correct this for consistency and clarity.
Additionally, configorTagImg is using rotatorCirclePng as its image source. If this is a placeholder, it should be replaced with the correct configuration icon.
| this.rect = new Konva.Rect({ | ||
| width: 20, | ||
| height: 20, | ||
| cornerRadius: 10, | ||
|
|
||
| shadowEnabled: true, | ||
| shadowColor: 'rgba(51, 51, 51, 0.2)', | ||
| shadowBlur: 4, | ||
| shadowOffsetY: 2, | ||
|
|
||
| stroke: 'rgba(217, 223, 229, 1)', | ||
| strokeWidth: 0.5, | ||
| fill: '#fff' | ||
| }) | ||
|
|
||
| this.image = new Konva.Image({ | ||
| width: 20, | ||
| height: 20, | ||
| cornerRadius: 10, | ||
| image: configorTagImg, | ||
| rotation: 0 | ||
| }) |
There was a problem hiding this comment.
The dimensions and corner radius are hardcoded here. These magic numbers are also implicitly used in the positioning logic within the update method. To improve maintainability and readability, it's recommended to define these values as constants. For example:
const BUTTON_SIZE = 20;
const BUTTON_RADIUS = 10;You can then use these constants both here for the button's shape and in the update method for positioning (e.g., this.width() / 2 - BUTTON_SIZE / 2).
| function toggleSpriteVisible() { | ||
| const name = props.sprite.name | ||
| const action = { name: { en: `Configure sprite ${name}`, zh: `修改精灵 ${name} 配置` } } |
There was a problem hiding this comment.
The action name Configure sprite ${name} is a bit generic for an action that only toggles visibility. A more descriptive name like Toggle visibility for sprite ${name} would make the user's action history clearer and more accurate.
const action = { name: { en: `Toggle visibility for sprite ${name}`, zh: `切换精灵 ${name} 的可见性` } }
| <slot name="img" :style="imgStyle"></slot> | ||
| <UIBlockItemTitle size="medium"> | ||
| {{ name }} | ||
| <template v-if="visible != null ? !visible : false" #suffix> |
There was a problem hiding this comment.
The condition v-if="visible != null ? !visible : false" is unnecessarily complex. It can be simplified to v-if="visible === false" for better readability, as it achieves the same result: the icon should only be shown when visible is explicitly false.
<template v-if="visible === false" #suffix>
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. |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
spx-gui/src/components/editor/common/viewer/quick-config/QuickConfig.vue
Show resolved
Hide resolved
| clearTimeout(timer) | ||
| activeInteractions++ | ||
| } | ||
| function handleInteractionEnd() { |
There was a problem hiding this comment.
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()
}
}…improved modularity
| & > div { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
消除 通过键盘移动 Sprite/Widget 带来的 焦点 默认行为
There was a problem hiding this comment.
消除 通过键盘移动 Sprite/Widget 带来的 焦点 默认行为
这个建议 comment 到代码里？
There was a problem hiding this comment.
已有的 zorder 相关的命名中都是把 zorder 当成一个单词而不是两个（z-order）的；这里组件的命名建议保持一致
| // TODO: Simple synchronization between fast-changing data and slow-changing data. | ||
| // This implementation cannot eliminate race conditions and may result in data overwriting. | ||
| // Improve when a better solution is available. | ||
| export function useSyncFastSlowValue<T>(fast: WatchSource<T>, slow: WatchSource<T>, cast: (value: T) => T = (v) => v) { |
There was a problem hiding this comment.
感觉这里的两个数据源的区别不在于 fast or slow，而在于一个是源头（model 上）的数据，一个是局部的编辑状态
类比一下的话，对于一个带 debounce 的 input，也会存在两个值，一个是外部传入的 prop value，一个内部实时的编辑值，后者经过 debounce 后才会同步到前者
这里类似，只不过 NodeTransformer + SpriteNode + QuickConfig 整体（其实就是 StageViewer 或 MapViewer）构成了这个“大的 input”。所以用类似的做法应该就可以，以 StageViewer 为例的话，应该就是：
- 在
StageViewer维护一份“临时的编辑状态”（它可能是 x, y，也可能是 size、heading），这跟你现在在 QuickConfig 这里做的类似，只不过它不应该是属于 QuickConfig 的逻辑 - 来自外部数据源（sprite）的状态 patch 上这份临时状态得到“最新状态”，
StageViewer向SpriteNode、QuickConfig等提供这份“最新状态”，并提供修改这份状态的接口 SpriteNode等不直接去修改外部数据源，而是通过StageViewer提供的接口修改，StageViewer会负责与外部数据源的同步
这样我感觉数据流会更清晰一些
There was a problem hiding this comment.
根据上面提到的，我把 patch 外部数据 和 接受数据变化 相关逻辑挪到了 StageViewer/MapViewer，NodeTransformer + SpriteNode + QuickConfig 目前只负责接受 props 和发送数据变化。
代码见：ec51326
我发现 SpriteNode 内部的 handleChange 也有一些直接修改 sprite 的行为，这些是否需要挪出去统一处理？
There was a problem hiding this comment.
我发现
SpriteNode内部的handleChange也有一些直接修改 sprite 的行为，这些是否需要挪出去统一处理？
嗯要挪就都挪出去比较合理
| false | ||
| ) | ||
| async function moveZorder(direction: keyof typeof moveActionNames) { |
There was a problem hiding this comment.
Nit: keyof typeof moveActionNames 这也太别扭了...可以考虑把这个 type 也定义下跟 moveActionNames 一起暴露出来
| </script> | ||
|
|
||
| <template> | ||
| <ConfigPanel v-radar="{ name: 'Rotation style control', desc: 'Control to set sprite rotation style' }"> |
| trigger="manual" | ||
| placement="top" | ||
| :visible="rotateDropdownVisible" | ||
| :disabled="sprite.rotationStyle === RotationStyle.None" |
There was a problem hiding this comment.
Nit: 如果对于非 normal 的 rotationStyle 也可能会渲染 HeadingConfigPanel 的话，这里条件用 sprite.rotationStyle !== RotationStyle.Normal 可能更合适？
| function toggleSpriteVisible() { | ||
| const name = props.sprite.name | ||
| const action = { name: { en: `Toggle visibility for sprite ${name}`, zh: `切换精灵 ${name} 的可见性` } } |
| v-radar="{ name: 'Visibility control', desc: 'Control to toggle sprite visibility' }" | ||
| @click="toggleSpriteVisible" | ||
| > | ||
| {{ $t({ en: `${sprite.visible ? 'Hide' : 'Show'} Sprite`, zh: `${sprite.visible ? '隐藏' : '显示'}精灵` }) }} |
There was a problem hiding this comment.
跟隔壁的其他 menu item 对应，文案没必要带上“Sprite/精灵”
| color: 'stage', | ||
| selectable: false | ||
| selectable: false, | ||
| visible: true |
There was a problem hiding this comment.
隔壁 UIEditorSpriteItem 的 visible 默认值是 null 而这里是 true，有什么说法吗？
There was a problem hiding this comment.
SpriteQuickConfig 跟 QuickConfig 的关系有点奇怪，一般来说前者应该是基于后者的封装才对..
| menuPos.value = { | ||
| x: stagePos.x + pointerPos.x, | ||
| y: stagePos.y + pointerPos.y + offsetY | ||
| function updateQuickConfigPos() { |
There was a problem hiding this comment.
🤣 这个逻辑比想象的要更复杂，有简化的空间吗？
如果不好简化，注释解释下这里定位的逻辑？
2 participants
Fixed #2513