Improve WPM pathing paint editor - #104
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: 6c4c9a5061
ℹ️ 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".
| while (stack.length) { | ||
| const index = stack.pop(); | ||
| const x = index % W, y = Math.floor(index / W); | ||
| paintCell(x, y, replacement); |
There was a problem hiding this comment.
When Fill targets a large uniform region, this synchronous loop invokes paintCell for every matching cell; each invocation updates the canvas with putImageData and adds an entry to gestureChanges, after which an object per cell is posted to the extension host. Real WPM grids can contain hundreds of thousands of cells, so filling an ordinary blank region can block the webview or exhaust memory. Accumulate the affected cells and refresh/post them in batches or a compact representation.
Useful? React with 👍 / 👎.
| /** The only WPM header version with a documented byte layout. */ | ||
| export const WPM_KNOWN_VERSION = 0; | ||
|
|
||
| export const WPM_FLAG_DEFS: readonly WpmFlagDefinition[] = [ |
There was a problem hiding this comment.
This new helper module has only one production consumer, wpmPreview.ts, so it creates a one-off abstraction solely for this viewer. Keep the definitions and color helpers in the existing WPM preview module rather than adding a separate utility file, as required by the repository's localization convention.
AGENTS.md reference: AGENTS.md:L43-L43
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43023546f5
ℹ️ 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".
| runs.forEach((run) => { | ||
| if (!Number.isInteger(run.start) || !Number.isInteger(run.length) || !Number.isInteger(run.value) || | ||
| run.start < 0 || run.length < 1 || run.start + run.length > data.length) return; | ||
| for (let index = run.start; index < run.start + run.length; index++) changes.push({ index, value: run.value }); |
There was a problem hiding this comment.
On a large uniform WPM region, each compact run is expanded back into one { index, value } object per cell before applyChanges, while collectWpmRunRequests similarly expands it into a per-cell Map; a million-cell fill therefore still creates millions of allocations and can block or exhaust the webview and extension host. Fresh evidence after the earlier flood-fill comment is that the new run protocol compacts only the message transport, then reconstructs the original per-cell representation at both endpoints; apply and retain the runs without this expansion.
Useful? React with 👍 / 👎.
| document.querySelectorAll('[data-brush-bit]').forEach((input) => input.addEventListener('change', updateBrushValue)); | ||
| const brushSizeInput = document.getElementById('brushSize'); | ||
| const brushSizeValue = document.getElementById('brushSizeValue'); | ||
| brushSizeInput.value = String(brushSize); |
There was a problem hiding this comment.
When VS Code recreates a webview whose saved brushSize is greater than 1, this updates the range input and the actual painting size but leaves #brushSizeValue at the hard-coded 1 × 1 until the user moves the slider. The editor can therefore display a 1-cell brush while applying an unexpectedly large restored brush; initialize the output here or call updateBrushSize() during setup.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90778ae382
ℹ️ 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".
| if (!gestureChanges.has(index)) gestureChanges.set(index, data[index]); | ||
| data[index] = next; | ||
| if (refresh) refreshCell(index); |
There was a problem hiding this comment.
With a large brush or a long Line gesture, every stamped cell calls refreshCell, which immediately invokes putImageData; a size-32 brush therefore performs 1,024 canvas uploads for a single point, including repeated uploads where adjacent stamps overlap, and a drag across a normal WPM can multiply this into hundreds of thousands of synchronous calls that freeze the webview. Update the backing pixels while stamping, then upload the image once per pointer update or completed line.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f03b49bc5
ℹ️ 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".
| let lineStart = null, lineCurrent = null; | ||
|
|
||
| function eventCell(e) { | ||
| const rect = viewport.getBoundingClientRect(); |
There was a problem hiding this comment.
When painting a large WPM, every pointer movement that reaches another cell calls paintLine, which calls this function and synchronously uploads the entire W × H image even if only a few pixels changed; a 1920 × 1920 grid copies roughly 14 MiB per event and can still freeze brush drags. Fresh evidence after the earlier brush-upload comment is that batching replaced the one-cell dirty upload with this unconditional full-image upload; retain one upload per pointer update, but pass the accumulated dirty bounds to putImageData.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7a3fbe281
ℹ️ 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".
| refreshCell(index); | ||
| } | ||
| }); | ||
| offCtx.putImageData(img, 0, 0); |
There was a problem hiding this comment.
On large WPM grids, every completed gesture and every undo/redo receives an applyRuns acknowledgement that reaches this unconditional full-image upload, even when the runs changed only one cell; it also leaves the dirty bounds set by refreshCell, so the next brush flush may upload the bounding rectangle of both edits. Fresh evidence after the earlier full-image-upload comment is that pointer updates now use flushImage(), but this acknowledgement path still copies the entire W × H image; call flushImage() here as well so the upload is bounded and the dirty state is reset.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Codex review loop complete: 6 review rounds were run. All actionable findings were addressed, the final review of 056a955 was clean, and the GitHub build check passes. Local validation: tsc, npm test, lint, and the WPM e2e spec pass; no merge performed. |
1 participant
What changed
.wpmpreview into a compact paint-style workspace with a side palette.Water/Unknownlabels with the documented WC3 pathing meanings, including an explicit version warning for nonzero WPM versions..impis intentionally not an editing target.Validation
npm test— passed.npx tsc -p . --noEmit— passed.npm run lint— passed with one pre-existing warning inwebpack.config.js(CopyFilePluginunused).npm run test:e2e— 70 passed.Notes
The WPM fixture examined locally is version 0 and uses
0x40for ordinary interior ground, so the UI now calls itNo Water / Unfloatablerather than presenting it as visible water. Nonzero versions are preserved but marked as unverified because only version 0 has a documented layout.