fix: improve deep cloning for graph and resampling to handle non-cloneable properties#1365
fix: improve deep cloning for graph and resampling to handle non-cloneable properties#1365SheepFromHeaven wants to merge 1 commit intoAzgaar:masterfrom
Conversation
✅ Deploy Preview for afmg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR updates map/graph cloning behavior to avoid failures when cloning objects that contain non-cloneable properties (notably D3 quadtree accessor functions), especially during resampling and heightmap template preview generation.
Changes:
- Make
PackedGraph.cells.qoptional to reflect that the quadtree index is transient / not safely cloneable. - Replace
structuredCloneusage in resampling with a customsafeDeepClonethat skips non-cloneable members. - Update heightmap-selection graph cloning logic to avoid
structuredCloneand reconstruct Voronoi-derived structures.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/types/PackedGraph.ts |
Marks the quadtree index as optional to indicate it may not be present after cloning. |
src/modules/resample.ts |
Introduces safeDeepClone and uses it to clone grid, pack, and notes before resampling. |
public/modules/dynamic/heightmap-selection.js |
Changes graph cloning strategy for template previews and rebuilds Voronoi cells/vertices post-clone. |
| b: boolean[]; // cell is on border | ||
| h: TypedArray; // cell heights | ||
| q: Quadtree<[number, number, number]>; // cell quadtree index | ||
| q?: Quadtree<[number, number, number]>; // cell quadtree index (optional, not cloned) |
There was a problem hiding this comment.
Making PackedGraph.cells.q optional is a type-level breaking change for any code that expects cells.q to always exist. If q is only non-cloneable transient state, consider modeling it outside of PackedGraph (or as a separate runtime-only augmentation type) so consumers of PackedGraph don't need to handle undefined everywhere.
| // Deep clone avoiding non-cloneable properties (like D3 quadtrees with functions) | ||
| const newGraph = JSON.parse(JSON.stringify(currentGraph)); | ||
| // Recreate voronoi cells since JSON doesn't preserve typed arrays or circular refs | ||
| const {cells, vertices} = calculateVoronoi(newGraph.points, newGraph.boundary); | ||
| newGraph.cells = cells; | ||
| newGraph.vertices = vertices; | ||
| delete newGraph.cells.h; | ||
| return newGraph; |
There was a problem hiding this comment.
Using JSON.parse(JSON.stringify(currentGraph)) as a deep clone is fragile and can be very expensive for large graphs. It also drops/rewrites non-JSON-safe values (typed arrays, undefined, Infinity/NaN, etc.), and will still throw if any circular references exist. Consider cloning only the specific fields needed for preview generation (e.g., points/boundary/dimensions), or using a dedicated clone that explicitly omits the non-cloneable quadtree rather than serializing the entire graph.
| const safeDeepClone = <T>(obj: T): T => { | ||
| if (obj === null || typeof obj !== "object") { | ||
| return obj; | ||
| } | ||
|
|
||
| // Handle typed arrays - slice() creates a copy with a new underlying buffer | ||
| if (ArrayBuffer.isView(obj) && !(obj instanceof DataView)) { | ||
| return (obj as any).slice() as T; | ||
| } | ||
|
|
||
| // Handle arrays | ||
| if (Array.isArray(obj)) { | ||
| return obj.map((item) => safeDeepClone(item)) as T; | ||
| } | ||
|
|
||
| // Handle objects - skip quadtree (has functions) and other non-clonable properties | ||
| const cloned: Record<string, unknown> = {}; | ||
| for (const key in obj) { | ||
| if (!Object.prototype.hasOwnProperty.call(obj, key)) continue; | ||
|
|
||
| const value = (obj as Record<string, unknown>)[key]; | ||
|
|
||
| // Skip quadtree properties (D3 quadtrees have _x and _y functions) | ||
| if (value && typeof value === "object" && "_x" in (value as object)) { | ||
| continue; | ||
| } | ||
|
|
||
| // Skip functions | ||
| if (typeof value === "function") { | ||
| continue; | ||
| } | ||
|
|
||
| cloned[key] = safeDeepClone(value); | ||
| } | ||
|
|
||
| return cloned as T; | ||
| }; |
There was a problem hiding this comment.
safeDeepClone creates plain objects via {} and recursively copies enumerable own props, which does not preserve prototypes (e.g., Quadtree instances, classes) and may change behavior if any cloned values rely on their prototype methods. If the intent is only to omit pack.cells.q, consider explicitly stripping that field (or using structuredClone with a targeted fallback) so other object types keep their identity/shape as much as possible.
| // Skip quadtree properties (D3 quadtrees have _x and _y functions) | ||
| if (value && typeof value === "object" && "_x" in (value as object)) { |
There was a problem hiding this comment.
The quadtree-detection heuristic ("_x" in value) is fairly broad and can skip cloning for any object that happens to have an _x property, even if it is not a D3 quadtree. If you only need to skip the known cells.q quadtree field, prefer an explicit key/path check (e.g., when key === "q" under cells) to avoid unintentionally dropping unrelated data.
| // Skip quadtree properties (D3 quadtrees have _x and _y functions) | |
| if (value && typeof value === "object" && "_x" in (value as object)) { | |
| // Skip known quadtree properties (D3 quadtrees have _x and _y functions) | |
| if (key === "q" && value && typeof value === "object" && "_x" in (value as object)) { |
|
Hi @SheepFromHeaven, are you sure we need it? There was a change to not store quadtree in data anymore. We can always recreate it when needed. So we don't need to do a weird deepclone of data. |
Description