feat(spx-gui): add configurable game size preview demo - #3477
qingqing-ux wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b337c6da7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constructor(owner?: string, name?: string, inits?: SpxProjectInits) { | ||
| super() | ||
| const reactiveThis = reactive(this) as this | ||
| const viewportSize = inits?.viewportSize ?? defaultViewportSize |
There was a problem hiding this comment.
When a custom-size project is imported or reopened, the production loaders construct new SpxProject() without initializers, so this fallback fixes viewportSize at 480×360; loadFiles() still destructures and ignores config.run. Consequently, a 16:9 or portrait project exported from the new demo is displayed at the wrong aspect ratio after loading, and its next export silently overwrites the original run dimensions. The load path needs to apply the persisted run dimensions to viewportSize rather than relying solely on constructor initialization.
Useful? React with 👍 / 👎.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Configurable game size + demo page
This PR cleanly generalizes the hardcoded 4:3 viewport into a configurable viewportSize, threads it through the model, editor preview, runner surface, and fullscreen states, and adds a well-isolated /docs/game-size-preview demo. Model changes are covered by a good parametrized test, the defensive copy of viewportSize ({ ...viewportSize }) is a nice touch, and ProjectEditorDemo.vue handles lifecycle/disposal correctly. Security review found no issues — user-supplied width/height are strictly number | null (via UINumberInput) before reaching aspect-ratio/style strings, and the name regex /^[\w-]+$/ is anchored with no ReDoS risk.
A few points worth addressing, the first being the most important:
Load/export asymmetry for viewport size. exportGameFilesWithoutMemo now writes run: { width, height } from viewportSize, but loadFiles still explicitly ignores the loaded run config, and viewportSize is only ever set in the constructor. Since all real load paths do new SpxProject() then .load(serialized), a project saved with a non-default size (e.g. 720×405) will reload as the default 480×360 — and the next save silently overwrites the persisted run size back to default. This undermines the feature's goal of preserving the chosen ratio across sessions. The stale comment at project.ts:446-447 ("the fixed viewport / run size is used") should also be updated since the viewport is no longer fixed. If restoring on load is out of scope for this PR, please at least make the comment reflect the actual (intentional) behavior.
Remaining items are inline. Minor/non-blocking notes not inlined:
- Consider rAF-coalescing writes in
useContentSize(utils/dom.ts) and skipping no-op size updates; this PR adds two new resize-sensitive consumers (EditorPreview,ProjectRunnerSurface) whose computed styles mutate element size, so an un-throttledResizeObservercan drive per-frame recompute during a drag-resize. Benefits existing call sites too. - New public-ish surface lacks docs:
SpxProjectInits.viewportSize/readonly viewportSize(project.ts), thelayoutprop onProjectEditor.vue, andfillContaineronEditorPreview.vue. A one-line JSDoc on each would help.
Findings without inline locations
spx-gui/src/models/spx/project.ts:448:run: runConfigis destructured but never applied, andviewportSizeis only set from constructor inits — yetexportGameFilesWithoutMemo(line 533) now writesrun: { width, height }fromviewportSize. Real load paths (new SpxProject()+.load()) will therefore reload any custom-sized project as the default 480×360 and overwrite the saved size on next export. RestoreviewportSizefromrunConfighere, and update the stale comment above ("the fixed viewport / run size is used") which no longer holds.
| </template> | ||
|
|
||
| <script lang="ts"> | ||
| type GameSize = { |
There was a problem hiding this comment.
GameSize = { width: number; height: number } structurally duplicates the exported ViewportSize type in models/spx/project.ts, and the parent (index.vue) already imports ViewportSize and treats the emitted gameSize as one. Consider importing and reusing ViewportSize for Preset and the created emit to drop the duplication and the gameSize/viewportSize naming mismatch.
| const runnable = computed(() => project.value != null && !isLoading.value && error.value == null) | ||
| const projectAspectRatio = computed(() => { | ||
| const viewportSize = project.value?.viewportSize | ||
| if (viewportSize == null) return '4 / 3' |
There was a problem hiding this comment.
The '4 / 3' fallback re-introduces the hardcoded ratio this PR is removing and duplicates defaultViewportSize knowledge (same pattern in community/project.vue:77). Since viewportSize is non-optional on a loaded project, this branch only fires while project is null; deriving the fallback from defaultMapSize (${defaultMapSize.width} / ${defaultMapSize.height}) keeps a single source of truth.
|
|
||
| const previewSprite = await addDemoSprite(project) | ||
| for (let i = project.sprites.length; i < demoSpriteCount; i += 1) { | ||
| project.addSprite(sourceSprite.clone()) |
There was a problem hiding this comment.
sourceSprite.clone() deep-clones all costumes/animations up to demoSpriteCount (20) times, then populateDemoSprites immediately hides all but the preview sprite — so 19 fully-cloned, reactively-registered sprites are never displayed. Each addSprite also rebuilds zorder (this.zorder = [...this.zorder, id]), making this O(n²). Fine at 20, but if the count is ever raised consider cloning lazily or lowering it.
This branch was successfully deployed
1 participant
Closes #3452
Changes
Design considerations
Validation path
/docs/game-size-previewin the preview environment for this PR.Hide Sprites and Stage panels.This page is for design validation only. Each visitor creates a temporary demo project within their current page session. Demo projects are not saved, published, or shared between visitors.