Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the simulation’s material/tool model to treat clearing cells as the 'empty' material (instead of a separate 'erase' tool), and adjusts UI and Playwright tests accordingly.
Changes:
- Adds
'empty'to the sharedMaterialunion and maps it to theEMPTYparticle ID. - Updates the UI tool list/colors/categories to use
'empty'in place of'erase'. - Updates Playwright tests’ dropdown interaction to click via mouse coordinates.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/particles.spec.ts | Changes dropdown opening to use boundingBox() + page.mouse.click() |
| tests/controls.spec.ts | Same dropdown-click approach for the controls test |
| src/sim/constants.ts | Adds 'empty' to Material and to MATERIAL_TO_ID |
| src/physics.worker.ts | Removes 'erase' tool handling and routes input tools through MATERIAL_TO_ID |
| src/App.tsx | Replaces 'erase' with 'empty' in UI types/config and tweaks pointer handling on the trigger |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const box = await trigger.boundingBox() | ||
| await page.mouse.click(box!.x + box!.width / 2, box!.y + box!.height / 2) |
There was a problem hiding this comment.
trigger.boundingBox() can return null (e.g., if the element isn't visible yet). The non-null assertion (box!) will make this test flaky/fail with a runtime error; add an explicit visibility assertion and a null check (or use await trigger.click() after ensuring visibility).
| const box = await trigger.boundingBox() | |
| await page.mouse.click(box!.x + box!.width / 2, box!.y + box!.height / 2) | |
| await expect(trigger).toBeVisible() | |
| await trigger.click() |
| const box = await trigger.boundingBox() | ||
| await page.mouse.click(box!.x + box!.width / 2, box!.y + box!.height / 2) |
There was a problem hiding this comment.
trigger.boundingBox() can return null; using box! can cause a runtime failure and flakiness. Add await expect(trigger).toBeVisible() and assert the bounding box is non-null before clicking (or prefer await trigger.click() if possible).
| const box = await trigger.boundingBox() | |
| await page.mouse.click(box!.x + box!.width / 2, box!.y + box!.height / 2) | |
| await expect(trigger).toBeVisible() | |
| await trigger.click() |
| @@ -70,7 +70,7 @@ function addParticles(cellX: number, cellY: number, tool: Material | 'erase', br | |||
| if (nx >= 0 && nx < cols && ny >= 0 && ny < rows) { | |||
| const idx = ny * cols + nx | |||
| const spawnRate = ARCHETYPES[matId]?.spawnRate ?? 0.45 | |||
| if ((tool === 'erase' || (rand() < spawnRate && grid[idx] === EMPTY))) { | |||
| if (rand() < spawnRate) { | |||
| grid[idx] = matId | |||
| } | |||
There was a problem hiding this comment.
Brush painting logic changed: with tool 'empty' the code now uses spawnRate (default 0.45 because ARCHETYPES[EMPTY] is null) and only clears a cell when rand() < spawnRate. Also, for all tools it now overwrites any existing particle because the prior grid[idx] === EMPTY guard was removed. If the intent is to make 'empty' act like erase and keep paint-from-empty behavior, handle EMPTY as a special case (always set to EMPTY), and for other materials keep the grid[idx] === EMPTY check (or otherwise prevent unintended overwrites).
| type Material = 'empty' | 'sand' | 'water' | 'dirt' | 'stone' | 'plant' | 'fire' | 'gas' | 'fluff' | 'bug' | 'plasma' | 'nitro' | 'glass' | 'lightning' | 'slime' | 'ant' | 'alien' | 'quark' | 'crystal' | 'ember' | 'static' | 'bird' | 'gunpowder' | 'tap' | 'anthill' | 'bee' | 'flower' | 'hive' | 'honey' | 'nest' | 'gun' | 'cloud' | 'acid' | 'lava' | 'snow' | 'volcano' | 'mold' | 'mercury' | 'void' | 'seed' | 'rust' | 'spore' | 'algae' | 'poison' | 'dust' | 'firework' | 'bubble' | 'glitter' | 'star' | 'comet' | 'blackhole' | 'firefly' | 'worm' | 'fairy' | 'fish' | 'moth' | 'vent' | ||
| type Tool = Material |
There was a problem hiding this comment.
Material is duplicated as a long union type in this file even though src/sim/constants.ts now exports type Material (including 'empty'). Duplicating this list risks the UI and worker diverging again; consider importing and reusing the shared Material type instead of redefining it here.
| key={m} | ||
| className={`material-dropdown-item ${tool === m ? 'active' : ''}`} | ||
| onClick={() => { if (m !== 'erase') lastMaterialRef.current = m as Material; setTool(m); setDropdownOpen(false) }} | ||
| onClick={() => { if (m !== 'empty') lastMaterialRef.current = m as Material; setTool(m); setDropdownOpen(false) }} |
There was a problem hiding this comment.
lastMaterialRef.current is written when selecting a non-'empty' tool, but it's never read anywhere in the file. This makes the intent unclear and adds dead state; either remove it or reintroduce the behavior that consumes it (e.g., toggling back to the last non-empty material).
| onClick={() => { if (m !== 'empty') lastMaterialRef.current = m as Material; setTool(m); setDropdownOpen(false) }} | |
| onClick={() => { setTool(m); setDropdownOpen(false) }} |
| e.preventDefault(); | ||
| (e.currentTarget as HTMLElement).setPointerCapture(e.pointerId) |
There was a problem hiding this comment.
Calling e.preventDefault() on onPointerDown for a <button> prevents the button from receiving focus via mouse/touch interaction, which can hurt keyboard navigation and assistive tech workflows. If the goal is only to suppress drag/scroll side effects, consider avoiding preventDefault here (or applying it only once a drag is detected) so the control can still be focused normally.
No description provided.