feat: save and load state of the creator #13
Conversation
WalkthroughThis update refactors asset management and rendering logic by expanding the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (integration-tests/index.ts)
participant Creator (initCreator)
participant ZigLogic (connectOnAssetUpdateCallback)
participant Renderer (runCreator)
User->>UI (integration-tests/index.ts): Selects image file
UI->>Creator: Calls addImage(id, img, points?)
Creator->>Renderer: Updates textures with {texture, url}
Creator->>ZigLogic: Notifies asset update via callback
ZigLogic->>Creator: Triggers onAssetsUpdate callback with serialized assets
Creator->>UI: Calls provided onAssetsUpdate(assets)
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/utils/getDefaultPoints.ts (1)
24-35: Consider edge case handling for zero-dimension images.The scaling logic is sound, but the function doesn't handle edge cases where the image might have zero width or height, which would result in division by zero.
Consider adding validation:
function getDefaultTextureScale(img: HTMLImageElement, canvas: HTMLCanvasElement) { + if (img.width === 0 || img.height === 0) { + return 1 // or throw an error + } + const heightDiff = canvas.height - img.height const widthDiff = canvas.width - img.widthsrc/WebGPU/pointer.ts (1)
20-20: Remove empty event listener or add implementation.The empty
mouseleaveevent listener serves no purpose and adds unnecessary overhead. Either implement the logic or remove it entirely.- canvas.addEventListener('mouseleave', () => {})If you need to track when the mouse leaves the canvas (e.g., to reset pointer state), consider implementing it:
- canvas.addEventListener('mouseleave', () => {}) + canvas.addEventListener('mouseleave', () => { + // Reset pointer state or handle mouse leave + })integration-tests/index.ts (1)
11-20: Consider uncommenting the example asset for more comprehensive testing.The commented-out serialized asset example could provide valuable test coverage for the asset loading functionality. Consider creating a test case that actually uses this asset.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 16-16: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
integration-tests/index.ts(1 hunks)src/WebGPU/pointer.ts(2 hunks)src/index.ts(2 hunks)src/logic/index.d.ts(2 hunks)src/logic/index.zig(2 hunks)src/run.ts(3 hunks)src/utils/getDefaultPoints.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/run.ts (2)
src/index.ts (1)
TextureSource(24-27)src/WebGPU/programs/initPrograms.ts (3)
drawTexture(16-16)drawTriangle(10-10)pickTexture(17-17)
integration-tests/index.ts (1)
src/index.ts (1)
initCreator(29-101)
src/utils/getDefaultPoints.ts (1)
src/utils/clamp.ts (1)
clamp(1-3)
src/index.ts (4)
src/run.ts (1)
runCreator(11-56)src/logic/index.d.ts (3)
connectOnAssetUpdateCallback(32-32)ASSET_ID_TRESHOLD(16-16)add_texture(18-18)src/WebGPU/getTexture/index.ts (1)
createTextureFromSource(106-117)src/utils/getDefaultPoints.ts (1)
getDefaultPoints(3-19)
🪛 GitHub Check: CodeQL
integration-tests/index.ts
[warning] 16-16: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
🔇 Additional comments (10)
src/utils/getDefaultPoints.ts (2)
3-19: LGTM! Well-structured utility function with proper scaling logic.The function correctly calculates default points for texture mapping with appropriate centering and scaling. The implementation properly handles the coordinate transformation from canvas space to UV coordinates.
28-31: Verify logic for determining dominant dimension.The condition
heightDiff < widthDiffdetermines whether to scale by height or width, but this logic might not always produce the expected result. WhenheightDiffis smaller, it means the image height is closer to canvas height, so scaling by height makes sense. However, consider if this aligns with the intended behavior.Would you like me to verify this logic with some test cases to ensure it produces the expected scaling behavior?
src/WebGPU/pointer.ts (1)
1-6: LGTM! Consistent formatting improvements.The formatting changes improve code consistency by using single quotes, proper indentation, and multi-line imports. Moving
updatePointerinside the function scope is appropriate since it's only used withininitMouseController.Also applies to: 14-18, 22-25, 39-42
integration-tests/index.ts (2)
8-23: LGTM! Proper integration with updated initCreator API.The changes correctly implement the new
initCreatorsignature with serialized assets and asset update callback. The empty array andconsole.logcallback are appropriate for testing purposes.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 16-16: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
27-27: Static analysis warning is a false positive.The CodeQL warning about "DOM text reinterpreted as HTML" on line 16 appears to be a false positive. Line 27 shows a standard type assertion for accessing
filesproperty from an input element, which is safe and doesn't involve HTML interpretation.src/run.ts (2)
6-6: LGTM! Proper type integration with TextureSource interface.The import and parameter type change correctly align with the new
TextureSourceinterface that includes both texture and URL information.Also applies to: 16-16
26-27: LGTM! Correct extraction of GPU texture from TextureSource wrapper.The changes properly access the
.textureproperty fromTextureSourceobjects, maintaining the existing WebGPU rendering functionality while supporting the enhanced texture metadata structure.Also applies to: 29-30
src/logic/index.d.ts (1)
9-13: Good addition of type definitions and callback mechanism.The new
Texturetype andconnectOnAssetUpdateCallbackfunction provide proper type safety for the asset update flow between Zig and TypeScript. The quote style change also improves consistency.Also applies to: 15-15, 32-32
src/index.ts (2)
73-84: Good validation and integration with the new texture management.The ID validation and integration with the getDefaultPoints utility function improves the robustness of the API.
15-27: Well-structured type definitions for the new asset management system.The
SerializedAssetandTextureSourcetypes provide clear interfaces for the expanded functionality, and the updated function signature properly reflects the new requirements.Also applies to: 29-33
| draw_texture: (vertex_data, texture_id) => | ||
| drawTexture(canvasPass, canvasMatrix, vertex_data.typedArray, textures[texture_id].texture), | ||
| draw_triangle: (vertex_data) => drawTriangle(canvasPass, canvasMatrix, vertex_data.typedArray), | ||
| pick_texture: (vertex_data, texture_id) => | ||
| pickTexture(pickPass, pickMatrix, vertex_data.typedArray, textures[texture_id]), | ||
| pickTexture(pickPass, pickMatrix, vertex_data.typedArray, textures[texture_id].texture), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding bounds checking for texture array access.
The code accesses textures[texture_id] without bounds checking. If texture_id is out of range, this will cause a runtime error.
Consider adding bounds validation:
connectWebGPUPrograms({
draw_texture: (vertex_data, texture_id) => {
+ if (texture_id >= textures.length) {
+ console.error(`Invalid texture_id: ${texture_id}`)
+ return
+ }
drawTexture(canvasPass, canvasMatrix, vertex_data.typedArray, textures[texture_id].texture)
},
pick_texture: (vertex_data, texture_id) => {
+ if (texture_id >= textures.length) {
+ console.error(`Invalid texture_id: ${texture_id}`)
+ return
+ }
pickTexture(pickPass, pickMatrix, vertex_data.typedArray, textures[texture_id].texture)
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| draw_texture: (vertex_data, texture_id) => | |
| drawTexture(canvasPass, canvasMatrix, vertex_data.typedArray, textures[texture_id].texture), | |
| draw_triangle: (vertex_data) => drawTriangle(canvasPass, canvasMatrix, vertex_data.typedArray), | |
| pick_texture: (vertex_data, texture_id) => | |
| pickTexture(pickPass, pickMatrix, vertex_data.typedArray, textures[texture_id]), | |
| pickTexture(pickPass, pickMatrix, vertex_data.typedArray, textures[texture_id].texture), | |
| connectWebGPUPrograms({ | |
| draw_texture: (vertex_data, texture_id) => { | |
| if (texture_id < 0 || texture_id >= textures.length) { | |
| console.error(`Invalid texture_id: ${texture_id}`) | |
| return | |
| } | |
| drawTexture( | |
| canvasPass, | |
| canvasMatrix, | |
| vertex_data.typedArray, | |
| textures[texture_id].texture | |
| ) | |
| }, | |
| draw_triangle: (vertex_data) => | |
| drawTriangle(canvasPass, canvasMatrix, vertex_data.typedArray), | |
| pick_texture: (vertex_data, texture_id) => { | |
| if (texture_id < 0 || texture_id >= textures.length) { | |
| console.error(`Invalid texture_id: ${texture_id}`) | |
| return | |
| } | |
| pickTexture( | |
| pickPass, | |
| pickMatrix, | |
| vertex_data.typedArray, | |
| textures[texture_id].texture | |
| ) | |
| }, | |
| }) |
🤖 Prompt for AI Agents
In src/run.ts around lines 26 to 30, the code accesses textures[texture_id]
without checking if texture_id is within the valid range, which can cause
runtime errors. Add bounds checking before accessing textures[texture_id] to
ensure texture_id is a valid index within the textures array. If texture_id is
invalid, handle the case gracefully, such as by returning early, throwing a
descriptive error, or using a default texture.
| declare module '*.zig' { | ||
| export const ASSET_ID_TRESHOLD: number | ||
| export const init_state: (width: number, height: number) => void | ||
| export const add_texture: (id: number, points: PointUV[], textre_index: number) => void |
There was a problem hiding this comment.
Fix the typo in the parameter name.
The parameter textre_index should be texture_index.
-export const add_texture: (id: number, points: PointUV[], textre_index: number) => void
+export const add_texture: (id: number, points: PointUV[], texture_index: number) => void📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const add_texture: (id: number, points: PointUV[], textre_index: number) => void | |
| export const add_texture: (id: number, points: PointUV[], texture_index: number) => void |
🤖 Prompt for AI Agents
In src/logic/index.d.ts at line 18, there is a typo in the parameter name
`textre_index`. Rename this parameter to `texture_index` to correct the
spelling.
| var on_asset_update_cb: *const fn ([]Texture) void = undefined; | ||
| pub fn connectOnAssetUpdateCallback(cb: *const fn ([]Texture) void) void { | ||
| on_asset_update_cb = cb; | ||
| } |
There was a problem hiding this comment.
Initialize the callback pointer to prevent undefined behavior.
The on_asset_update_cb is declared as undefined, which could cause a crash if on_pointer_up is called before connectOnAssetUpdateCallback.
Consider initializing with a no-op function or checking for undefined before calling:
-var on_asset_update_cb: *const fn ([]Texture) void = undefined;
+var on_asset_update_cb: ?*const fn ([]Texture) void = null;Then update the invocation to check for null:
-if (result.len > 0) {
- on_asset_update_cb(result);
-}
+if (result.len > 0 and on_asset_update_cb != null) {
+ on_asset_update_cb.?(result);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var on_asset_update_cb: *const fn ([]Texture) void = undefined; | |
| pub fn connectOnAssetUpdateCallback(cb: *const fn ([]Texture) void) void { | |
| on_asset_update_cb = cb; | |
| } | |
| // src/logic/index.zig | |
| // Change the callback pointer to an optional initialized to null | |
| -var on_asset_update_cb: *const fn ([]Texture) void = undefined; | |
| +var on_asset_update_cb: ?*const fn ([]Texture) void = null; | |
| pub fn connectOnAssetUpdateCallback(cb: *const fn ([]Texture) void) void { | |
| on_asset_update_cb = cb; | |
| } | |
| // … | |
| // In on_pointer_up (around lines 86–97), guard against a null callback before calling | |
| - if (result.len > 0) { | |
| - on_asset_update_cb(result); | |
| - } | |
| + if (result.len > 0 and on_asset_update_cb != null) { | |
| + on_asset_update_cb.?(result); | |
| + } |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 21 to 24, the callback pointer
on_asset_update_cb is initialized as undefined, which risks undefined behavior
if called before being set. Fix this by initializing on_asset_update_cb to a
no-op function that matches its signature or by adding a check before invoking
it to ensure it is not undefined. This prevents crashes by ensuring the callback
pointer is always valid or safely skipped.
| var result = std.heap.page_allocator.alloc(Texture, state.assets.count()) catch unreachable; | ||
| var iterator = state.assets.iterator(); | ||
| var i: usize = 0; | ||
| while (iterator.next()) |entry| { | ||
| result[i] = entry.value_ptr.*; | ||
| i += 1; | ||
| } | ||
|
|
||
| if (result.len > 0) { | ||
| on_asset_update_cb(result); | ||
| } |
There was a problem hiding this comment.
Fix memory leak - allocated array is never freed.
The array allocated with std.heap.page_allocator.alloc is never deallocated, causing a memory leak on every pointer up event.
Since the callback likely needs to process the data asynchronously, consider either:
- Documenting that the callback is responsible for freeing the memory
- Using a different allocation strategy (e.g., arena allocator)
- Keeping a persistent buffer that gets reused
For immediate fix, if the callback processes synchronously, add deallocation:
if (result.len > 0) {
on_asset_update_cb(result);
}
+defer std.heap.page_allocator.free(result);However, this won't work if the callback stores the reference. Consider passing ownership to the JavaScript side or using a different memory management approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = std.heap.page_allocator.alloc(Texture, state.assets.count()) catch unreachable; | |
| var iterator = state.assets.iterator(); | |
| var i: usize = 0; | |
| while (iterator.next()) |entry| { | |
| result[i] = entry.value_ptr.*; | |
| i += 1; | |
| } | |
| if (result.len > 0) { | |
| on_asset_update_cb(result); | |
| } | |
| var result = std.heap.page_allocator.alloc(Texture, state.assets.count()) catch unreachable; | |
| var iterator = state.assets.iterator(); | |
| var i: usize = 0; | |
| while (iterator.next()) |entry| { | |
| result[i] = entry.value_ptr.*; | |
| i += 1; | |
| } | |
| if (result.len > 0) { | |
| on_asset_update_cb(result); | |
| } | |
| defer std.heap.page_allocator.free(result); |
🤖 Prompt for AI Agents
In src/logic/index.zig around lines 87 to 97, the array allocated with
std.heap.page_allocator.alloc is not freed, causing a memory leak. To fix this,
if the on_asset_update_cb callback processes the data synchronously and does not
retain the reference, add a call to free the allocated array immediately after
the callback returns. If the callback stores the reference asynchronously,
update the design to either transfer ownership to the caller or use a persistent
or arena allocator to manage the buffer lifecycle properly.
| assets.forEach((asset) => { | ||
| const img = new Image() | ||
| img.src = asset.url | ||
| img.onload = () => { | ||
| addImage(asset.id, img, asset.points) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add error handling for image loading failures.
If an image fails to load, the error is silently ignored, which could lead to missing assets without any indication to the user.
assets.forEach((asset) => {
const img = new Image()
img.src = asset.url
img.onload = () => {
addImage(asset.id, img, asset.points)
}
+ img.onerror = (error) => {
+ console.error(`Failed to load asset ${asset.id} from ${asset.url}:`, error)
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assets.forEach((asset) => { | |
| const img = new Image() | |
| img.src = asset.url | |
| img.onload = () => { | |
| addImage(asset.id, img, asset.points) | |
| } | |
| }) | |
| assets.forEach((asset) => { | |
| const img = new Image() | |
| img.src = asset.url | |
| img.onload = () => { | |
| addImage(asset.id, img, asset.points) | |
| } | |
| img.onerror = (error) => { | |
| console.error(`Failed to load asset ${asset.id} from ${asset.url}:`, error) | |
| } | |
| }) |
🤖 Prompt for AI Agents
In src/index.ts around lines 86 to 92, the image loading process lacks error
handling for failures. Add an onerror event handler to the Image object that
logs or handles the error appropriately when an image fails to load, ensuring
that loading failures are detected and reported instead of being silently
ignored.
| connectOnAssetUpdateCallback((serializedData: Texture[]) => { | ||
| const serializedAssetsTextureUrl = [...serializedData].map<SerializedAsset>((asset) => ({ | ||
| id: asset.id, | ||
| points: [...asset.points].map((point) => ({ | ||
| x: point.x, | ||
| y: point.y, | ||
| u: point.u, | ||
| v: point.v, | ||
| })), | ||
| url: textures[asset.texture_id].url, | ||
| })) | ||
| onAssetsUpdate(serializedAssetsTextureUrl) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the deep copy of serialized data.
The spread operators and map functions create unnecessary copies. Since you're transforming the data anyway, the explicit copying is redundant.
-connectOnAssetUpdateCallback((serializedData: Texture[]) => {
- const serializedAssetsTextureUrl = [...serializedData].map<SerializedAsset>((asset) => ({
- id: asset.id,
- points: [...asset.points].map((point) => ({
- x: point.x,
- y: point.y,
- u: point.u,
- v: point.v,
- })),
- url: textures[asset.texture_id].url,
- }))
- onAssetsUpdate(serializedAssetsTextureUrl)
-})
+connectOnAssetUpdateCallback((serializedData: Texture[]) => {
+ const serializedAssetsTextureUrl = serializedData.map<SerializedAsset>((asset) => ({
+ id: asset.id,
+ points: asset.points,
+ url: textures[asset.texture_id].url,
+ }))
+ onAssetsUpdate(serializedAssetsTextureUrl)
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connectOnAssetUpdateCallback((serializedData: Texture[]) => { | |
| const serializedAssetsTextureUrl = [...serializedData].map<SerializedAsset>((asset) => ({ | |
| id: asset.id, | |
| points: [...asset.points].map((point) => ({ | |
| x: point.x, | |
| y: point.y, | |
| u: point.u, | |
| v: point.v, | |
| })), | |
| url: textures[asset.texture_id].url, | |
| })) | |
| onAssetsUpdate(serializedAssetsTextureUrl) | |
| }) | |
| connectOnAssetUpdateCallback((serializedData: Texture[]) => { | |
| const serializedAssetsTextureUrl = serializedData.map<SerializedAsset>((asset) => ({ | |
| id: asset.id, | |
| points: asset.points, | |
| url: textures[asset.texture_id].url, | |
| })) | |
| onAssetsUpdate(serializedAssetsTextureUrl) | |
| }) |
🤖 Prompt for AI Agents
In src/index.ts around lines 59 to 71, the current code uses spread operators
and map functions to create deep copies of serializedData and its nested points,
which is redundant because you are already transforming the data. Simplify the
code by directly mapping over serializedData and points without spreading them
into new arrays, as the transformation itself produces new objects and no
explicit deep copy is needed.
|
🎉 This PR is included in version 0.1.0-next.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Refactor
Chores