Conversation
There was a problem hiding this comment.
Pull request overview
This PR defers compression/ExpressionJSON conversion for large frontend objects to reduce CPU load during CreateFrontEndObject, avoid symbol-context corruption during ExpressionJSON serialization, and includes a Graphics3D interaction fix (plus a small README addition).
Changes:
- Introduce deferred (“on-demand”) compression for large frontend objects and ensure compression wrappers are released when syncing/fetching objects.
- Adjust
Graphics3DDOM structure and add handling for aControlsoption (including disabling pointer interaction). - Add a “Star History” section to the README and update the 3.0.3 release notes content.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a Star History embed section. |
| modules/wljs-graphics3d-threejs/src/kernel.js | Wraps Graphics3D DOM in a container; updates layout offset logic; adds Controls disabling logic; updates destroy cleanup. |
| modules/wljs-graphics3d-threejs/dist/kernel.js | Built output reflecting the Graphics3D changes. |
| modules/wljs-editor/src/FrontendObjectSync.wl | Releases deferred compression wrappers before resolving fetched frontend objects. |
| modules/wljs-editor/src/FrontendObject.wl | Implements deferred compression wrapper + release path; updates FrontEndRef/GetObject to handle wrappers. |
| modules/wljs-demos-archive/Demos/Release notes/3.0.3.wln | Updates release notes notebook data (including a new “Misc” section mentioning Graphics3D/controls). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| With[{promise = data["Promise"], kernel = GenericKernel`HashMap[ data["Kernel"] ]}, | ||
| (* [FIXME] Include these symbols normally using Needs[] *) | ||
| (* we release any possible deferred compression wrappers *) | ||
| With[{result = CoffeeLiqueur`Extensions`FrontendObject`Internal`Objects[data["UId"] ]["Public"]}, | ||
| With[{c = CoffeeLiqueur`Extensions`FrontendObject`Internal`releaseCompression[result]}, | ||
| GenericKernel`Async[kernel, EventFire[promise, Resolve, c ] ]; |
There was a problem hiding this comment.
FetchFrontEndObject now calls CoffeeLiqueurExtensionsFrontendObjectInternalreleaseCompression, but this package file doesn’t declare a dependency on CoffeeLiqueurExtensionsFrontendObject`` (no BeginPackage[..., {"CoffeeLiqueurExtensionsFrontendObject", ...}] / Needs[...]). If Objects is present via restored notebook state but the FrontendObject package wasn’t loaded, releaseCompression will be undefined and you’ll resolve the promise with an unevaluated wrapper. Add an explicit dependency/Needs for the FrontendObject package before using releaseCompression (and then the FIXME can be removed).
| FrontEndRef[uid_String] := If[KeyExistsQ[Objects, uid], | ||
| Objects[uid, "Private"] // ReleaseHold | ||
| With[{o = Objects[uid, "Private"]}, | ||
| releaseCompression[o] (*Ehhhh Okay... we need to release it anyway *) | ||
| ] // ReleaseHold |
There was a problem hiding this comment.
FrontEndRef now routes stored objects through releaseCompression, which (for deferred wrappers) performs an ExportByteArray[..., "ExpressionJSON"] + compress, then ReleaseHold immediately triggers Compressed[...] and re-imports/decompresses the JSON. This adds a costly JSON round-trip on every FrontEndRef call and can reintroduce the symbol-context issues described in the header comment (since ExpressionJSON conversion happens under the current $Context). Consider unwrapping the deferred wrapper for kernel-side access (e.g., return the original expr/Hold[expr]) and only run ExpressionJSON conversion when actually sending over the link.
| compression; | ||
|
|
||
| (* [NOTE] This is a deferred compression method, i.e. it is only applied when the object is requested via net / link *) | ||
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where forntend object was created, *) |
There was a problem hiding this comment.
Typo in comment: “forntend” should be “frontend”.
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where forntend object was created, *) | |
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where frontend object was created, *) |
|
|
||
| (* [NOTE] This is a deferred compression method, i.e. it is only applied when the object is requested via net / link *) | ||
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where forntend object was created, *) | ||
| (* this leads to some symbols to be falsly assumed to be in Global`, which will throw errors on the frontend *) |
There was a problem hiding this comment.
Typo in comment: “falsly” should be “falsely”.
| (* this leads to some symbols to be falsly assumed to be in Global`, which will throw errors on the frontend *) | |
| (* this leads to some symbols to be falsely assumed to be in Global`, which will throw errors on the frontend *) |
| (* [NOTE] This is a deferred compression method, i.e. it is only applied when the object is requested via net / link *) | ||
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where forntend object was created, *) | ||
| (* this leads to some symbols to be falsly assumed to be in Global`, which will throw errors on the frontend *) | ||
| (* The only way to avoid this is to deffer compression and ExpressionJSON convertion. *) |
There was a problem hiding this comment.
Typo in comment: “deffer” should be “defer”.
| (* The only way to avoid this is to deffer compression and ExpressionJSON convertion. *) | |
| (* The only way to avoid this is to defer compression and ExpressionJSON convertion. *) |
| if ('Controls' in options && !(await interpretate(options.Controls))) { | ||
| controlObject.disabled = true; | ||
| domElement.style.pointerEvents = 'none'; | ||
| } |
There was a problem hiding this comment.
When Controls is explicitly set to a falsy value, you disable pointer events on domElement, but controlObject.init(...) is still called unconditionally later (and controls = controlObject.o assumes init ran). This still constructs OrbitControls and registers listeners even though controls are intended to be disabled. Consider gating the init call (and controls assignment) behind the same enabled/disabled condition, or explicitly set controls.enabled = false / configure OrbitControls’ enableRotate|enablePan|enableZoom depending on what "Controls" is meant to control (the current pointerEvents = 'none' disables all interactions, not just rotation/pan).
| controlObject.o.update(); | ||
| }, | ||
| init: (camera, dom) => { | ||
| controlObject.o = new OrbitControls( camera, domElement ); |
There was a problem hiding this comment.
controlObject.init takes (camera, dom) but ignores the dom parameter and instead closes over the outer domElement variable. This makes the API misleading and can break if callers pass a different DOM element (e.g., for different control implementations). Use the dom parameter inside init (or remove it from the signature) to keep the control-object interface consistent.
| controlObject.o = new OrbitControls( camera, domElement ); | |
| controlObject.o = new OrbitControls( camera, dom ); |
| (* [NOTE] This is a deferred compression method, i.e. it is only applied when the object is requested via net / link *) | ||
| (* Otherwise ExpressionJSON uncontrollably lifts the context from symbols depending where forntend object was created, *) | ||
| (* this leads to some symbols to be falsly assumed to be in Global`, which will throw errors on the frontend *) | ||
| (* The only way to avoid this is to deffer compression and ExpressionJSON convertion. *) |
There was a problem hiding this comment.
Typo in comment: “convertion” should be “conversion”.
| (* The only way to avoid this is to deffer compression and ExpressionJSON convertion. *) | |
| (* The only way to avoid this is to deffer compression and ExpressionJSON conversion. *) |
This reduces the load on CPU when calling
CreateFrontEndObjectand potentially a memory footprint.In particular it solves an issue with
ExpressionJSONuncontrollably converting context of compressed symbols to whatever context is set to$Context. This causes many issues in isolated modules using runtime generated symbols.also fixes Graphics3D panning issue