Fixes (#6) #8
Fixes (#6) #8
Conversation
…laycanvas#8121) * Workaround for packHalf2x16 precision issues on some Android devices * tabs --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…flows (playcanvas#8128) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* fix: fixed enablePan logic * fix: reworked pan logic to specify desktop Pan and double touch
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…laycanvas#8129) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* Optimize BoundingBox.containsPoint performance * lint --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…canvas#8140) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…canvas#8138) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…aycanvas#8146) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* fix: removed snap dependency on enabled or attached state * fix: clarify snapping state description in TransformGizmo
* fix: rotate not working with shift in fly mode * fix: increase max speed limits in controls from 10 to 100
* fix: added option to filter button events for gizmos * fix: correct wording in gizmo mouse button interaction documentation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
… points (playcanvas#8154) * Picker class can (optional) render depth and provide world space pick points * updated agents.md * agents --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…vas#8172) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* [Fix] Correct transmission attenuation rendering * lint --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* Add loadingCount parameter to GSplat frame:ready event * removed unused internal function --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…playcanvas#8182) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…h mipmap-based prefix sums (playcanvas#8255) * Adds a GPU-based radix sort implementation using fragment shaders with mipmap-based prefix sums * lint * test * reverted test --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…reating compute shaders (playcanvas#8258) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
…playcanvas#8259) Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com>
* Refactor GitHub Actions workflow for npm publishing * fix: consistent .yml extensions * fix: standardize YAML formatting in workflows
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughModernization and feature updates: GitHub Actions upgraded, React Router moved to v6 across examples, many gfxOptions shader URLs removed, observer-driven refactors for blend-tree examples, new Gaussian-splatting examples and WGSL shaders, build/tooling and dependency bumps, misc. fixes and .gitignore/renovate adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary of ChangesHello @JaclynCodes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on maintenance and security enhancements. It introduces CodeQL for static analysis to identify potential vulnerabilities and updates several key development dependencies to their latest stable versions. Additionally, it refines the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consists of dependency updates in the package-lock.json files for both the root project and the examples directory. These changes include a security update for serialize-javascript, along with maintenance and minor version bumps for other packages like js-yaml and glob. The modifications, including the addition of "peer": true flags, are consistent with standard npm package management and appear correct and beneficial for the project's health and security.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (23)
.gitignore (1)
2-2: Consolidate redundant Snap Cursor ignore patterns. 🫧Line 2 already ignores the entire
.cursordirectory recursively, making the specific patterns on lines 19–20 redundant. Since the parent directory is ignored, everything nested within it is automatically excluded from version control.Consider simplifying to just line 2, unless the specific patterns serve a documentation purpose:
🔎 Proposed simplification
*.DS_Store .cursor .idea/ .vscode/ build tree*.*.html coverage docs examples/dist examples/node_modules node_modules npm-debug.log types stats.html .npmrc examples/.npmrc .prettierrc -# Added by Snap Cursor Rules extension -.cursor/rules/cursor-rules-debug.log -.cursor/rules/remoteAlso applies to: 18-20
examples/iframe/loader.mjs (1)
173-174: Excellent fix for the frame = 0 edge case! 🫧This correctly handles the scenario where the app has already started with
frame = 0. The old truthiness check would've treated0as falsy, incorrectly bypassing the immediate_appStart()call and queueing a listener instead. The explicit!== undefinedcheck fixes that nicely.The comment is clear and helpful. If you'd like extra type safety, consider
typeof this._app.frame === 'number', though the current approach is perfectly sound given PlayCanvas'sframeproperty is typed asnumber | undefined.Optional: Add type guard for extra robustness
- // Check if app has already started (frame is a number, including 0) - if (this._app.frame !== undefined) { + // Check if app has already started (frame is a number, including 0) + if (typeof this._app.frame === 'number') { this._appStart();This ensures
frameis actually a number, not just defined. However, it's purely defensive—PlayCanvas should only ever setframetonumber | undefined.examples/src/app/components/ErrorBoundary.mjs (1)
86-89: Fragment removal is spot on! Though the jsx() pattern is a bit... unconventional. 🫧The elimination of the fragment wrapper makes perfect sense—no need for extra wrapping when returning a single element. The code is cleaner and more direct.
That said, the manual
jsx()function call is a curious choice for React 19. The more idiomatic approach would be standard JSX syntax:return <Label id="errorLabel" text="RENDER FAILED" />;If the custom
jsxutility (imported on Line 4) serves a specific purpose in the examples architecture, this pattern makes sense. Otherwise, consider aligning with standard React conventions.🔎 Optional refactor to standard JSX syntax
- return jsx(Label, { - id: 'errorLabel', - text: 'RENDER FAILED' - }); + return <Label id="errorLabel" text="RENDER FAILED" />;Note: Only apply this if the custom
jsxutility isn't required for the examples framework.examples/src/examples/gaussian-splatting/lod-streaming.controls.mjs (1)
47-64: Consider removing the hardcoded default or matching the platform-specific logic.The fallback value
'4M'here doesn't align with the mobile default ('1M') set in the example file at line 148. Sinceobserver.get('splatBudget')should already have the correct platform-specific value from the example initialization, you can safely rely on it without a fallback. If you prefer a fallback, consider making it conditional or documenting why desktop's default is chosen for the UI.🔎 Proposed fix
jsx(SelectInput, { type: 'string', binding: new BindingTwoWay(), link: { observer, path: 'splatBudget' }, - value: observer.get('splatBudget') || '4M', + value: observer.get('splatBudget'), options: [ { v: 'none', t: 'No limit' }, { v: '1M', t: '1M' }, { v: '2M', t: '2M' }, { v: '3M', t: '3M' }, { v: '4M', t: '4M' }, { v: '6M', t: '6M' } ] })examples/src/examples/gaussian-splatting/lod-streaming.example.mjs (1)
1-1: TheNO_MINISTATScomment might perplex future readers. 🫧Since MiniStats is explicitly instantiated at line 136 with custom configuration, this directive could be misleading. If it's a build-time flag to prevent default MiniStats while allowing custom instances, consider adding a clarifying comment. Otherwise, verify whether this directive is still needed.
examples/src/examples/camera/multi.controls.mjs (1)
65-90: Consider addingstepandprecisionfor consistency. 🫧These three movement speed sliders (moveSpeed, moveFastSpeed, moveSlowSpeed) don't specify
steporprecisionattributes, while other sliders in this file (such as rotateSpeed, zoomSpeed, and the damping controls) do.Adding these attributes would provide more consistent UX and clearer feedback to users about the granularity of their adjustments.
🔎 Example refactor
jsx(SliderInput, { binding: new BindingTwoWay(), link: { observer, path: 'attr.moveSpeed' }, min: 1, - max: 100 + max: 100, + step: 1, + precision: 0 })Apply similar changes to
moveFastSpeedandmoveSlowSpeedsliders.examples/src/examples/compute/edge-detect.example.mjs (1)
40-45: Consider removing unused ScriptComponentSystem. 🫧The
ScriptComponentSystemis included in the component systems, but this example doesn't appear to use any script components. Unlike other examples that use the Rotator script, this example handles camera movement directly in the update loop (lines 227-231).🔎 Proposed cleanup
createOptions.componentSystems = [ pc.RenderComponentSystem, pc.CameraComponentSystem, - pc.LightComponentSystem, - pc.ScriptComponentSystem + pc.LightComponentSystem ];examples/src/examples/gaussian-splatting/flipbook.example.mjs (1)
33-33: Modernize orbit camera to use ESM scripts and CameraControls.Lines 33–36 register ScriptComponentSystem and ScriptHandler, which remain active PlayCanvas systems. However, the orbit camera implementation (lines 93–102) uses legacy
camera.script.create()API with classic scripts. Modernize this to use CameraControls from 'playcanvas/scripts/esm/camera-controls.mjs' and ESM Scripts, which use modern ES Module syntax and provide the recommended way to write PlayCanvas scripts with better code organization and improved bundling.examples/scripts/build-thumbnails.mjs (1)
227-229: Document the rationale forshell: true🫧Adding
{ shell: true }to the spawn call is a functional change that likely addresses cross-platform compatibility (particularly for Windows environments where shell wrappers are sometimes needed fornpxcommands).While the security implications are minimal here—since all arguments are hardcoded constants with no user input—this option does change how the command is executed and could have subtle effects on process handling and cleanup.
Consider adding a brief inline comment explaining why this option was added (e.g., "Required for npx on Windows" or "Fixes spawn issues on certain platforms").
Suggested documentation
- const server = spawn(cmd, ['serve', 'dist', '-l', PORT, '--no-request-logging', '--config', '../serve.json'], { - shell: true - }); + // shell: true needed for cross-platform npx compatibility + const server = spawn(cmd, ['serve', 'dist', '-l', PORT, '--no-request-logging', '--config', '../serve.json'], { + shell: true + });examples/assets/scripts/misc/hatch-material.mjs (1)
92-153: Excellent WGSL vertex shader translation! 🫧The vertexWGSL implementation is a faithful and well-structured translation of the GLSL version. The shader correctly:
- Uses engine-provided #include directives for shader chunks (transformCoreVS, normalCoreVS) with proper WGSL syntax
- Declares attributes, uniforms, and varyings with appropriate type annotations
- Implements lighting calculations that match the GLSL logic
- Accesses uniforms via the
uniform.prefix consistently- Handles attribute access in a manner consistent with the engine's include pattern
PlayCanvas supports WGSL for WebGPU platforms, and the simplified WGSL syntax used here is officially supported by the engine. Consider verifying that this shader has been tested in a WebGPU-enabled environment to confirm correct rendering behaviour across target devices.
examples/src/examples/gaussian-splatting/crop.controls.mjs (1)
5-5: Consider removing the unusedReactparameter. 🧹The
Reactparameter is destructured but never referenced in the function body. The actual React functionality is handled through thejsxandfragmenthelpers.🔎 Proposed refactor
-export const controls = ({ observer, ReactPCUI, React, jsx, fragment }) => { +export const controls = ({ observer, ReactPCUI, jsx, fragment }) => {examples/src/examples/gaussian-splatting/crop.example.mjs (4)
6-6: Consider adding error handling for the dynamic import. 🛡️The dynamic import of
GsplatCropShaderEffectlacks error handling. If the script fails to load, the example will crash without a helpful message.🔎 Proposed enhancement
-const { GsplatCropShaderEffect } = await fileImport(`${rootPath}/static/scripts/esm/gsplat/shader-effect-crop.mjs`); +let GsplatCropShaderEffect; +try { + ({ GsplatCropShaderEffect } = await fileImport(`${rootPath}/static/scripts/esm/gsplat/shader-effect-crop.mjs`)); +} catch (error) { + console.error('Failed to load GsplatCropShaderEffect:', error); + throw error; +}
54-56: Consider adding error handling for asset loading. 🎯The asset loader lacks error handling. While examples may tolerate this, catching load failures would improve debugging experience.
🔎 Optional enhancement
-assetListLoader.load(() => { +assetListLoader.load(() => { + // Assets loaded successfully app.start(); +}, (error) => { + console.error('Failed to load assets:', error); });
58-66: Pause state isn't synchronized with the observer. ⏸️The
pausedvariable is toggled locally but never synchronized back to the data store viadata.set('paused', paused). This means external UI or other observers won't see the current pause state.🔎 Proposed fix
// Handle pause/play toggle data.on('togglePause', () => { paused = !paused; + data.set('paused', paused); });
114-123: The polling pattern lacks a maximum retry limit. 🔄The
checkMaterialfunction polls indefinitely every 100ms until the material is available. If the material never becomes available (due to load failure or configuration issue), this will poll forever, potentially impacting performance.🔎 Proposed enhancement with retry limit
// Wait for material to be available, then set initial state + const maxRetries = 50; // 5 seconds max + let retryCount = 0; const checkMaterial = () => { const material = getMaterial(); if (material) { updatePreciseDefine(data.get('precise')); - } else { + } else if (retryCount < maxRetries) { + retryCount++; setTimeout(checkMaterial, 100); + } else { + console.warn('Material not available after maximum retries'); } }; checkMaterial();examples/src/examples/animation/blend-trees-2d-directional.example.mjs (1)
184-191: Minor: Missing type annotation for consistencyThe cartesian example at line 183 uses
/** @type {any} */ animNodein the map callback, but this file omits it. Consider adding for consistency across examples:🔎 Suggested fix
const updateAnimPoints = () => { - const points = locomotionLayer._controller._states.Travel.animations.map(animNode => ({ + const points = locomotionLayer._controller._states.Travel.animations.map((/** @type {any} */ animNode) => ({ x: animNode.point?.x ?? 0, y: animNode.point?.y ?? 0, weight: animNode.weight ?? 0 })); data.set('data.animPoints', points); };examples/src/examples/animation/blend-trees-2d-directional.controls.mjs (1)
75-85: Touch event support missing (inconsistent with class component version)This functional component only handles
mousemove/mousedown(lines 84-85), while the class component inblend-trees-2d-cartesian.controls.mjshandles both mouse and touch events (touchmove,touchstartat lines 107-108).If touch support is desired for consistency:
🔎 Suggested fix to add touch support
const mouseEvent = (e) => { - if (e.buttons) { + if (e.targetTouches) { + const rect = canvas.getBoundingClientRect(); + const x = ((e.targetTouches[0].clientX - rect.left) / (width / 2)) - 1; + const y = -(((e.targetTouches[0].clientY - rect.top) / (height / 2)) - 1); + observer.set('data.pos', { x, y }); + } else if (e.buttons) { const rect = canvas.getBoundingClientRect(); const x = ((e.clientX - rect.left) / (width / 2)) - 1; const y = -(((e.clientY - rect.top) / (height / 2)) - 1); observer.set('data.pos', { x, y }); } }; canvas.addEventListener('mousemove', mouseEvent); canvas.addEventListener('mousedown', mouseEvent); + canvas.addEventListener('touchmove', mouseEvent); + canvas.addEventListener('touchstart', mouseEvent); return () => { canvas.removeEventListener('mousemove', mouseEvent); canvas.removeEventListener('mousedown', mouseEvent); + canvas.removeEventListener('touchmove', mouseEvent); + canvas.removeEventListener('touchstart', mouseEvent); observer.off('*:set', drawPosition); };examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs (1)
70-87: Variable shadowing: localwidth/heightshadow class gettersAt lines 73-74, local
const width = 8andconst height = 8shadow the class getter propertiesthis.widthandthis.height. This works but is confusing. Consider renaming todotWidth/dotHeightfor clarity (matching the directional functional component pattern at lines 37-38).🔎 Suggested fix
animPoints.forEach((animNode) => { const posX = (animNode.x + 1) * halfWidth; const posY = (animNode.y * -1 + 1) * halfHeight; - const width = 8; - const height = 8; + const dotWidth = 8; + const dotHeight = 8; ctx.fillStyle = '#ffffff80'; ctx.beginPath(); ctx.arc(posX, posY, halfWidth * 0.5 * animNode.weight, 0, 2 * Math.PI); ctx.fill(); ctx.fillStyle = '#283538'; ctx.beginPath(); - ctx.moveTo(posX, posY - height / 2); - ctx.lineTo(posX - width / 2, posY); - ctx.lineTo(posX, posY + height / 2); - ctx.lineTo(posX + width / 2, posY); + ctx.moveTo(posX, posY - dotHeight / 2); + ctx.lineTo(posX - dotWidth / 2, posY); + ctx.lineTo(posX, posY + dotHeight / 2); + ctx.lineTo(posX + dotWidth / 2, posY); ctx.closePath(); ctx.fill(); });examples/package.json (1)
41-43: Update component imports to align with React Router v7 conventions. 🫧The examples are already compatible with React 19.2.0 and React Router 7.9.6. However, to follow v7 best practices, migrate imports from
"react-router-dom"to"react-router"in Sidebar.mjs, MainLayout.mjs, Example.mjs, and index.mjs.While the backward-compatibility re-export works, the unified package structure reflects v7's streamlined architecture and prepares the codebase for any future enhancements.
examples/src/app/components/Example.mjs (2)
312-314: Curious wrapper pattern withdisplay: contents🫧Both
renderPortraitandrenderLandscapenow wrap their returns indivelements withdisplay: contentsstyling. While this works (the divs become layout-transparent), it's not immediately clear why this wrapper was introduced for these internal render methods.Could these divs be removed, or is there a specific React Router v6 or React 19 requirement driving this? If they're unnecessary, returning the JSX directly would be cleaner.
If there's no technical requirement for these wrappers, consider simplifying:
💭 Potential simplification
renderPortrait() { const { collapsed, show, files, description } = this.state; - return jsx( - 'div', - { style: { display: 'contents' } }, + return jsx( + fragment, + null, jsx( Panel,Also applies to: 375-377
418-422: Resolve TypeScript suppressions with proper typing 🫧The wrapper pattern is solid and follows React Router v6 best practices! However, the two
@ts-ignorecomments indicate type definition gaps that should be addressed:
- Line 420:
paramstype fromuseParams()could be properly typed- Line 421: The
matchprop structure could be properly defined in thePropstypeConsider adding proper type definitions:
🔎 Proposed typing improvements
+/** + * Route parameters for the Example route. + */ +type ExampleParams = { + category: string; + example: string; +}; + /** * Wrapper component to provide router params to the class component. * @returns {JSX.Element} The Example component with router params. */ function ExampleWithRouter() { - const params = useParams(); - // @ts-ignore + const params = useParams<ExampleParams>(); return jsx(Example, { match: { params } }); }The
Propstype definition at lines 40-42 already expectsmatch.paramswithcategoryandexample, so this should align perfectly.examples/src/app/components/MainLayout.mjs (1)
70-96: React Router v6 migration executed properly! 🫧The route structure correctly implements v6 patterns:
Routescomponent wraps all routes ✓Navigatewithreplace: truehandles the default redirect ✓elementprop properly renders components ✓However, the wrapper
divwithid="appInner-router"anddisplay: contents(lines 79-80) appears to follow the same pattern seen inExample.mjs. If theappInner-routerID isn't referenced elsewhere for CSS targeting or programmatic access, this wrapper could potentially be simplified to a fragment.Worth verifying if this wrapper serves a specific purpose:
#!/bin/bash # Check if appInner-router ID is referenced elsewhere rg -n 'appInner-router' --type css --type js --type ts -g '!*.mjs'examples/src/app/components/Sidebar.mjs (1)
120-120: Potential routing inconsistency with globallocation.hash🫧Line 120 uses the global
location.hashdirectly rather thanthis.props.location.hashfrom the router. While this may work for the initial load scenario described in the comment, it's inconsistent with the rest of the migration to router-provided location.Consider updating for consistency:
🔎 Suggested alignment with router location
// when first opening the examples browser via a specific example, scroll it into view // @ts-ignore if (!window._scrolledToExample) { - const examplePath = location.hash.split('/'); + const examplePath = this.props.location.pathname.split('/'); document.getElementById(`link-${examplePath[1]}-${examplePath[2]}`)?.scrollIntoView(); // @ts-ignore window._scrolledToExample = true; }Note: Using
pathnameinstead ofhashsince the router now tracks paths viapathnameinHashRouter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
examples/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (46)
.github/renovate.json(1 hunks).github/workflows/beta.yml(2 hunks).github/workflows/ci.yml(2 hunks).github/workflows/publish.yml(2 hunks).github/workflows/upload.yml(2 hunks).gitignore(2 hunks)AGENTS.md(1 hunks)examples/assets/models/jet-fighter.txt(1 hunks)examples/assets/scripts/misc/hatch-material.mjs(2 hunks)examples/iframe/loader.mjs(1 hunks)examples/package.json(1 hunks)examples/rollup.config.mjs(1 hunks)examples/scripts/build-thumbnails.mjs(3 hunks)examples/src/app/components/ErrorBoundary.mjs(2 hunks)examples/src/app/components/Example.mjs(4 hunks)examples/src/app/components/MainLayout.mjs(2 hunks)examples/src/app/components/Sidebar.mjs(5 hunks)examples/src/examples/animation/blend-trees-1d.example.mjs(1 hunks)examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs(6 hunks)examples/src/examples/animation/blend-trees-2d-cartesian.example.mjs(3 hunks)examples/src/examples/animation/blend-trees-2d-directional.controls.mjs(3 hunks)examples/src/examples/animation/blend-trees-2d-directional.example.mjs(3 hunks)examples/src/examples/animation/component-properties.example.mjs(1 hunks)examples/src/examples/animation/events.example.mjs(1 hunks)examples/src/examples/animation/layer-masks.example.mjs(1 hunks)examples/src/examples/animation/locomotion.example.mjs(1 hunks)examples/src/examples/animation/tween.example.mjs(1 hunks)examples/src/examples/camera/first-person.example.mjs(1 hunks)examples/src/examples/camera/fly.example.mjs(1 hunks)examples/src/examples/camera/multi.controls.mjs(3 hunks)examples/src/examples/camera/multi.example.mjs(1 hunks)examples/src/examples/camera/orbit.example.mjs(1 hunks)examples/src/examples/compute/edge-detect.compute-shader.wgsl(1 hunks)examples/src/examples/compute/edge-detect.example.mjs(1 hunks)examples/src/examples/compute/histogram.example.mjs(1 hunks)examples/src/examples/compute/particles.example.mjs(1 hunks)examples/src/examples/compute/texture-gen.example.mjs(1 hunks)examples/src/examples/compute/vertex-update.example.mjs(1 hunks)examples/src/examples/gaussian-splatting/crop.controls.mjs(1 hunks)examples/src/examples/gaussian-splatting/crop.example.mjs(1 hunks)examples/src/examples/gaussian-splatting/flipbook.example.mjs(1 hunks)examples/src/examples/gaussian-splatting/global-sorting.example.mjs(0 hunks)examples/src/examples/gaussian-splatting/lod-streaming.controls.mjs(1 hunks)examples/src/examples/gaussian-splatting/lod-streaming.example.mjs(5 hunks)examples/src/examples/gaussian-splatting/lod.controls.mjs(0 hunks)examples/src/examples/gaussian-splatting/lod.example.mjs(0 hunks)
💤 Files with no reviewable changes (3)
- examples/src/examples/gaussian-splatting/lod.controls.mjs
- examples/src/examples/gaussian-splatting/global-sorting.example.mjs
- examples/src/examples/gaussian-splatting/lod.example.mjs
🧰 Additional context used
🧬 Code graph analysis (19)
examples/src/examples/animation/layer-masks.example.mjs (2)
examples/src/examples/graphics/post-effects.example.mjs (1)
gfxOptions(33-37)examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/gaussian-splatting/lod-streaming.example.mjs (3)
examples/src/examples/gaussian-splatting/flipbook.example.mjs (2)
msOptions(73-73)camera(85-85)examples/src/examples/gaussian-splatting/lod-streaming-sh.example.mjs (3)
preset(146-146)gs(143-143)camera(157-157)examples/src/examples/gaussian-splatting/lod-instances.example.mjs (3)
preset(106-106)gs(148-148)camera(154-154)
examples/src/examples/compute/texture-gen.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/events.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/blend-trees-2d-directional.controls.mjs (1)
examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs (10)
controls(7-124)controls(7-124)React(8-8)animPoints(56-56)pos(57-57)ctx(59-59)width(73-73)height(74-74)halfWidth(60-60)halfHeight(61-61)
examples/src/examples/animation/locomotion.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/gaussian-splatting/flipbook.example.mjs (1)
examples/src/examples/gaussian-splatting/lod-streaming.example.mjs (5)
app(36-36)assets(99-108)assetListLoader(110-110)msOptions(119-119)miniStats(136-136)
examples/src/examples/gaussian-splatting/crop.example.mjs (1)
src/framework/app-base.js (1)
dt(2069-2069)
examples/src/examples/camera/first-person.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/component-properties.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/app/components/MainLayout.mjs (1)
examples/src/examples/gaussian-splatting/viewer.example.mjs (1)
orientation(183-183)
examples/src/examples/gaussian-splatting/crop.controls.mjs (1)
examples/src/examples/graphics/clustered-spot-shadows.example.mjs (1)
observer(8-8)
examples/src/examples/compute/histogram.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/compute/edge-detect.example.mjs (1)
src/framework/app-base.js (1)
dt(2069-2069)
examples/src/examples/animation/blend-trees-2d-cartesian.example.mjs (3)
examples/src/examples/animation/blend-trees-2d-directional.example.mjs (4)
gfxOptions(24-26)updateAnimPoints(184-191)points(185-189)modelEntity(94-96)examples/src/examples/animation/component-properties.example.mjs (1)
gfxOptions(14-16)examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/blend-trees-1d.example.mjs (15)
examples/src/examples/graphics/model-outline.example.mjs (1)
gfxOptions(10-14)examples/src/examples/graphics/area-picker.example.mjs (1)
gfxOptions(17-19)examples/src/examples/graphics/post-effects.example.mjs (1)
gfxOptions(33-37)examples/src/examples/animation/blend-trees-2d-cartesian.example.mjs (1)
gfxOptions(26-28)examples/src/examples/animation/blend-trees-2d-directional.example.mjs (1)
gfxOptions(24-26)examples/src/examples/animation/layer-masks.example.mjs (1)
gfxOptions(26-28)examples/src/examples/animation/component-properties.example.mjs (1)
gfxOptions(14-16)examples/src/examples/animation/events.example.mjs (1)
gfxOptions(17-19)examples/src/examples/graphics/clustered-omni-shadows.example.mjs (1)
gfxOptions(31-33)examples/src/examples/graphics/clustered-spot-shadows.example.mjs (1)
gfxOptions(22-24)examples/src/examples/graphics/dithered-transparency.example.mjs (1)
gfxOptions(20-25)examples/src/examples/graphics/lights-baked.example.mjs (1)
gfxOptions(7-9)examples/src/examples/graphics/lights.example.mjs (1)
gfxOptions(41-43)examples/src/examples/graphics/shadow-cascades.example.mjs (1)
gfxOptions(19-21)examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/camera/fly.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/tween.example.mjs (1)
examples/iframe/utils.mjs (2)
deviceType(106-106)deviceType(106-106)
examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs (2)
examples/src/examples/animation/blend-trees-2d-directional.controls.mjs (5)
controls(5-94)controls(5-94)canvas(10-10)animPoints(23-23)pos(24-24)examples/src/examples/misc/editor.controls.mjs (2)
controls(7-220)controls(7-220)
🪛 LanguageTool
AGENTS.md
[uncategorized] ~187-~187: Possible missing preposition found.
Context: ...ments help with quick understanding - Complex algorithms: Explain the approach and ...
(AI_HYDRA_LEO_MISSING_OF)
[grammar] ~188-~188: It appears that a hyphen is missing in the plural noun “to-dos”?
Context: ...h and any non-obvious optimizations - TODOs: Include issue reference or context ...
(TO_DO_HYPHEN)
[style] ~264-~264: Consider using a different verb for a more formal wording.
Context: ...ide errors or suppress warnings without fixing the underlying issue - **Fix at the sou...
(FIX_RESOLVE)
[duplication] ~324-~324: Possible typo: you repeated a word.
Context: ...s, ask to add documentation to the User Manual - Manual pages are Markdown files in the docs/...
(ENGLISH_WORD_REPEAT_RULE)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
321-321: Bare URL used
(MD034, no-bare-urls)
322-322: Bare URL used
(MD034, no-bare-urls)
323-323: Bare URL used
(MD034, no-bare-urls)
326-326: Bare URL used
(MD034, no-bare-urls)
327-327: Bare URL used
(MD034, no-bare-urls)
328-328: Bare URL used
(MD034, no-bare-urls)
329-329: Bare URL used
(MD034, no-bare-urls)
🔍 Remote MCP
GitHub Actions Migration & Security
Checkout Action v5→v6 Update:
The v6 upgrade updates persist-credentials to store credentials under $RUNNER_TEMP instead of directly in local git config, requiring a minimum Actions Runner version of v2.329.0 to access persisted credentials for Docker container action scenarios. This is a functional security improvement but may require runner updates in self-hosted environments.
Upload Artifact v5→v6:
The upload/download artifact action bumps align with current GitHub Actions standards and include security patches. [::web_search::]
React Router v5→v6 Migration Analysis
Migration Approach Used in PR:
The PR implements replacement of withRouter HOCs with hooks across example components (Example.mjs, Sidebar.mjs, MainLayout.mjs). This aligns with React Router v6 best practices.
Key Hook Usage:
- useParams() replaces v5's props.match.params
- useLocation() replaces v5's props.location
- Routes and Navigate replace Switch and Redirect components; route element prop replaces component prop
Critical Breaking Changes in Migration:
React Router v6 introduces several breaking changes: the Switch component has been removed and developers must use Routes instead; the syntax for defining routes has changed, and some props have been renamed or removed.
The PR's approach (using wrappers like ExampleWithRouter and SideBarWithRouter to inject hooks) is a valid intermediate strategy, though official recommendations suggest using the backwards compatibility package (react-router-dom-v5-compat) to upgrade apps incrementally for projects with more than a few routes. [::web_search::]
Dependency Updates
js-yaml, glob, serialize-javascript bumps: Minor version increments that represent patch/minor updates rather than major upgrades, indicating routine maintenance and security patches. No breaking changes noted.
React Package Upgrades (examples/package.json):
Updated to React 19 and React Router v7+ (per rollup.config change), requiring React 16.8+ for React Router v6, which uses hooks extensively. React 19 meets this requirement.
Observer Pattern Implementation
Multiple example files introduce observer-based data binding (blend-trees-2d-cartesian, blend-trees-2d-directional, lod-streaming). This represents a pattern shift from direct PlayCanvas entity manipulation to reactive data flow. The controls functions now accept an observer parameter to maintain two-way binding with centralized state rather than managing component state locally.
Script System & Shader URL Removal
Systematic removal of:
glslangUrlandtwgslUrlfrom gfxOptions across ~20+ examplesScriptComponentSystemandScriptHandlerfrom component/resource pipelines
This suggests deprecation or consolidation of external shader tooling in favor of internally-managed defaults or WGSL-based compute shaders (evidenced by new .wgsl files).
PR Documentation Gaps
Critical review concern: The PR description contains placeholder text ("Brief description of what this PR does") and an empty "Fixes #" line, indicating incomplete PR documentation that should reference the associated issue/PR #6. Checklist items for code standards and PR focus remain unchecked.
[::web_search::], [::Linear_search_documentation::]
🔇 Additional comments (64)
examples/assets/models/jet-fighter.txt (1)
1-5: Attribution file looks ship-shape! 🫧The file provides proper copyright attribution for the Mitsubishi F-2 model with a direct link to the source, and the CC BY 4.0 license is correctly referenced with an active link. The format is clear and human-readable.
However, I noticed an inconsistency: the PR's AI-generated summary describes comprehensive modernization work (GitHub Actions, React Router v6 migration, shader updates, observer pattern refactoring, etc.), yet this file is a simple model attribution document. This attribution file may be supporting the new Gaussian splatting examples mentioned in the PR objectives, but it's worth verifying that this fits the stated scope.
Could you confirm this file aligns with the project's standard format for model attributions? Also, does this addition belong in the scope described in the PR objectives?
examples/src/examples/animation/component-properties.example.mjs (1)
14-16: Shader URL cleanup looks ship-shape! 🫧The removal of
glslangUrlandtwgslUrlfromgfxOptionsaligns beautifully with the PR-wide pattern of consolidating shader tooling into internal engine defaults. This simplification is a win for maintainability.Based on the external context indicating systematic deprecation across 20+ examples, this change appears intentional and well-coordinated. The graphics device initialization should handle this gracefully with built-in defaults.
examples/src/app/components/ErrorBoundary.mjs (1)
4-4: Custom jsx import is an intentional architectural pattern and fully React 19 compatible. 🫧This custom jsx utility is not a misconfiguration—it's a consistent design choice across the entire examples application. The
jsx.mjsmodule provides a lightweight wrapper that aliasesReact.createElementand also exports afragmenthelper. All components in the app (Sidebar, MainLayout, Example, CodeEditor variants, and others) use this same import pattern, making it an intentional stylistic and architectural decision rather than a React 19 compatibility concern.examples/src/examples/gaussian-splatting/lod-streaming.example.mjs (4)
118-135: LGTM! The GSplats tracking adds valuable performance visibility. 🫧The MiniStats configuration properly converts splat counts to millions with appropriate precision and watermarking. This pairs nicely with the new budgeting controls.
148-148: Nice platform-aware default! 🫧The conditional splat budget (1M for mobile, 4M for desktop) is a sensible performance optimization that matches device capabilities.
179-193: The budget mapping logic looks solid. 🫧The function correctly translates UI presets to numeric splat budgets and wires up the observer pattern consistently with the LOD preset handling above. The fallback to
0(unlimited) is reasonable given the controlled UI inputs, though you might consider logging unexpected values during development for easier debugging.
200-200: Verify the tone mapping change is intentional.Switching from
ACEStoLINEARsignificantly alters the visual presentation—LINEAR provides no tone curve, while ACES applies cinematic highlight roll-off. For gaussian splats, LINEAR may preserve data accuracy better, but please confirm this change aligns with the intended look and doesn't create visual inconsistencies with other examples.examples/src/examples/camera/multi.controls.mjs (1)
65-70: Inconsistency detected: max range differs significantly from comparable controls 🫧The max value of 100 for camera movement speeds in multi.controls.mjs is inconsistent with the equivalent sliders in fly.controls.mjs and orbit.controls.mjs, both of which cap out at 10. This represents a 10x difference in maximum possible speed multiplier.
Since all three controls manage the same speed attributes using the camera-controls implementation (which applies speed values directly as movement multipliers), this difference warrants clarification. Either:
- The max should be 10 for consistency, or
- There's a specific reason for multi.controls to allow 10x higher speeds that should be documented
examples/src/examples/animation/events.example.mjs (1)
17-19: Graphics options simplified consistently 🫧The
gfxOptionssimplification aligns with the PR-wide pattern of removing external shader tool URLs. This change looks correct for standard rendering examples.examples/src/examples/animation/locomotion.example.mjs (1)
36-38: LGTM! 🫧Graphics options simplified consistently with the rest of the PR. The animation systems will continue to work with default shader handling.
examples/src/examples/camera/fly.example.mjs (1)
16-18: Consistent simplification 🫧The graphics options change is consistent with the PR-wide shader tooling consolidation.
examples/src/examples/camera/multi.example.mjs (1)
16-18: Graphics configuration simplified appropriately 🫧This change is part of the consistent PR-wide removal of explicit shader tooling URLs.
examples/src/examples/compute/particles.example.mjs (1)
19-21: Another compute example requiring verification 🫧Like the vertex-update example, this particles simulation relies on WGSL compute shaders (line 84). The removal of explicit shader tooling URLs should be verified to ensure the compute pipeline continues to function correctly.
Please test that particle simulation initializes and runs properly with the simplified graphics configuration, especially the storage buffer operations and compute dispatches.
examples/src/examples/camera/first-person.example.mjs (1)
20-22: Configuration change looks good 🫧The simplified
gfxOptionsis consistent with the PR-wide pattern. Standard rendering and physics systems will work with default shader handling.examples/src/examples/animation/tween.example.mjs (1)
14-16: Final file: change is consistent 🫧This completes the PR-wide simplification of graphics options. The tween animation example will work correctly with default shader handling.
examples/src/examples/compute/vertex-update.example.mjs (1)
22-24: WGSL compute shaders work natively without shader transpiler URLs 🫧The removal of
glslangUrlandtwgslUrlis correct for this example. GLSL shaders require transpilation to WGSL using a WASM transpiler on WebGPU, but native WGSL avoids this transpilation step. Since this example uses WGSL with automatic group and binding assignment managed by the engine, the shader tooling URLs are unnecessary. The simplifiedgfxOptionsconfiguration aligns with PlayCanvas's pattern for WebGPU-only examples.examples/src/examples/compute/histogram.example.mjs (1)
23-25: LGTM! Clean simplification of graphics options. 🫧The removal of
glslangUrlandtwgslUrlaligns with the broader repo-wide cleanup. This simplifies device initialization while preserving the essentialdeviceTypesconfiguration.examples/src/examples/camera/orbit.example.mjs (1)
16-18: LGTM! Consistent with the gfxOptions cleanup pattern. 🫧The removal of external shader loader URLs simplifies the graphics device configuration appropriately.
examples/src/examples/compute/texture-gen.example.mjs (1)
20-22: LGTM! Consistent cleanup. 🫧The gfxOptions simplification removes the external shader tool URLs while maintaining the necessary device type configuration.
examples/src/examples/animation/layer-masks.example.mjs (3)
23-23: LGTM! Bloom asset cleanup. 🫧The removal of the bloom script asset aligns with the script-based postprocessing deprecation across the PR.
26-28: LGTM! Consistent gfxOptions simplification. 🫧External shader tool URLs removed, maintaining only the essential
deviceTypesconfiguration.
38-49: LGTM! Complete script system removal. 🫧The removal of
ScriptComponentSystemandScriptHandleris consistent with the bloom postprocessing deprecation. Since this example no longer uses any script-based features, this cleanup is appropriate.examples/src/examples/animation/blend-trees-1d.example.mjs (3)
19-19: LGTM! Bloom asset cleanup. 🫧Consistent removal of the bloom script asset as part of the script-based postprocessing deprecation.
22-24: LGTM! Clean gfxOptions simplification. 🫧External shader loader URLs removed appropriately.
32-43: LGTM! Script system cleanup. 🫧Removal of
ScriptComponentSystemandScriptHandleris consistent with the deprecation of script-based postprocessing. This example no longer requires script capabilities.AGENTS.md (1)
1-342: Excellent addition! Comprehensive guidelines for AI agents and developers. 🫧This document provides valuable standards for code style, architecture, documentation, and contribution practices. The hierarchical module structure (core → platform → scene → framework) and the emphasis on performance-critical considerations are particularly well-articulated.
A few optional refinements based on static analysis:
- Line 53: Consider adding a language identifier to the fenced code block (e.g., ```text)
- Line 188: Style preference for "to-dos" instead of "TODOs"
- Lines 321-329: Consider formatting bare URLs as Markdown links for better rendering
Note regarding PR documentation: The PR description itself contains placeholder text ("Brief description of what this PR does") and an empty "Fixes #" line. As per the guidelines in this very document (section 22), PR descriptions should be complete and reference associated issues. Consider updating the PR description to align with these standards.
examples/src/examples/compute/edge-detect.example.mjs (4)
31-33: LGTM! Consistent with the gfxOptions cleanup. 🫧The new example follows the simplified pattern without external shader loader URLs.
113-141: LGTM! Well-structured MSAA render target creation. 🫧The function correctly creates a single-sample texture and configures the render target with optional MSAA support. The approach of resolving MSAA buffers to the color buffer is appropriate for compute shader input.
159-176: LGTM! Proper compute shader setup. 🫧The bind group format correctly defines the input texture with sampler and output storage texture. The
hasSampler: trueparameter on line 171 ensures proper binding slot allocation for the sampler.
233-240: LGTM! Correct workgroup dispatch calculation. 🫧The use of
Math.ceilensures all pixels are covered when dimensions aren't exact multiples of the 8×8 workgroup size. The shader properly guards against out-of-bounds access (line 11-13 of the WGSL file).examples/src/examples/compute/edge-detect.compute-shader.wgsl (1)
1-56: LGTM! Excellent Sobel edge detection implementation. 🫧The shader correctly implements:
- Proper 3×3 neighborhood sampling with bounds checking
- Standard ITU-R BT.601 luminance weights (0.299, 0.587, 0.114) for grayscale conversion
- Accurate Sobel operator kernels (gx and gy)
- Euclidean magnitude calculation for edge strength
- Appropriate blending of original color with red edge highlights
The workgroup size of 8×8 aligns well with the dispatch calculations in the example, and the bounds check (lines 11-13) prevents out-of-range writes when dimensions aren't exact multiples of the workgroup size.
examples/src/examples/gaussian-splatting/flipbook.example.mjs (2)
106-172: Lovely helper function and entity setup! 🫧The
createFlipbookEntityhelper nicely encapsulates the repetitive setup logic, and the three flipbook instances demonstrate different playback speeds effectively. The optional chaining on line 169 is a thoughtful touch to guard against potential undefined access.
72-82: VRAM monitoring is a sensible addition for Gaussian splat examples.The MiniStats configuration appropriately tracks texture VRAM usage, which is relevant for performance monitoring in splatting scenarios. The eslint-disable comment correctly acknowledges the intentionally unused variable reference.
examples/assets/scripts/misc/hatch-material.mjs (1)
198-247: Spot-on WGSL fragment shader implementation! ✨The fragmentWGSL block is a clean and accurate translation with proper WGSL idioms:
- Correctly separates texture and sampler declarations (lines 211-212) for WebGPU's binding model
- Uses
textureSample()with the array index as a distinct parameter rather than embedding it in coordinates—the right approach for texture_2d_array- Maintains the TOON preprocessor conditional logic
- Preserves the post-processing pipeline (fog, tone mapping, gamma correction) with engine includes
The translation from GLSL's
texture(uDiffuseMap, vec3(..., floor(level)))to WGSL'stextureSample(uDiffuseMap, uDiffuseMapSampler, ..., i32(floor(level)))is exactly how it should be done. Nicely handled! 🫧examples/src/examples/gaussian-splatting/crop.controls.mjs (2)
6-8: LGTM! Clean component setup. ✨The component destructuring and fragment usage follow established patterns for the observer-based UI architecture.
9-35: Nicely structured observer bindings! 🫧The two-way bindings for
preciseandedgeScale, combined with the event-driventogglePause, create a clean reactive interface. The slider constraints (min: 0.1, max: 1.0, precision: 2) provide sensible bounds for edge scaling.examples/src/examples/gaussian-splatting/crop.example.mjs (5)
11-19: Good graphics optimization! 🎨Disabling antialiasing for gaussian splats is a sensible performance optimization, and capping
maxPixelRatioat 2 prevents unnecessary resolution overhead on high-DPI displays.
42-47: Excellent resource cleanup! 🧼The resize listener is properly removed on app destruction, preventing memory leaks. This is exactly the kind of diligent cleanup we love to see.
130-160: Camera and post-processing setup looks solid! 📸The orbit camera configuration with low inertia (0.2) and focused framing on the hotel entity provides smooth interaction. The subtle bloom effect (intensity: 0.03) enhances the gaussian splats without being overbearing.
203-211: Elegant sine-based animation! 🌊The AABB animation uses a clever sine-based easing that creates a smooth, organic "breathing" effect. The asymmetric scaling (1.5× in XZ directions) adds visual interest while maintaining the vertical crop bounds.
215-215: Perfect! Exports the app for external integration. 🎁This export enables the controls module to bind to the app's observer, completing the reactive UI pattern.
examples/src/examples/animation/blend-trees-2d-cartesian.example.mjs (2)
182-189: Accessing internal PlayCanvas API_controller._statesThis reaches into PlayCanvas's private animation controller internals. While functional for visualization, it's fragile and may break with engine updates. 🫧
The pattern is consistent with
blend-trees-2d-directional.example.mjs(which accessesTravelstate), so this appears intentional for the examples. Just a heads-up that this relies on undocumented internals.
175-200: Observer integration looks solid! ✨The reactive data flow is well-structured:
- Initial values align with
animStateGraphDatadefaults (posX: -0.5,posY: 0.5)- Event listener properly propagates position changes to animation parameters
- Animation points update on every position change for accurate visualization
One small note: the observer subscription at line 195 isn't cleaned up, but since this example runs until page unload, it's acceptable here.
examples/src/examples/animation/blend-trees-2d-directional.example.mjs (1)
177-202: Observer integration is consistent with the cartesian example 🎯Good implementation that mirrors the pattern in
blend-trees-2d-cartesian.example.mjs:
- Initial values
{ x: 0, y: 0 }correctly match the state graph defaultsupdateAnimPointshelper properly extracts animation state for visualization- Event subscription triggers animation parameter updates
The ordering is correct—observer wiring happens before
modelEntityis added to the scene (line 204).examples/src/examples/animation/blend-trees-2d-directional.controls.mjs (1)
22-67: Drawing logic looks good! 🎨Clean implementation with proper fallbacks (
|| []and|| { x: 0, y: 0 }). The coordinate transformation logic correctly maps observer data to canvas coordinates with Y-axis inversion.examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs (1)
7-9: Observer integration via class component — functional approach also validThe class component approach works well here, with
observercaptured in closure scope. The directional controls use a functional component with hooks instead. Both patterns are valid, though the functional approach inblend-trees-2d-directional.controls.mjsis more modern React idiom.Consistency between the two might be nice eventually, but not blocking. 🫧
.github/workflows/beta.yml (2)
7-7: Quote style normalized for consistency.The change from single to double quotes aligns with formatting standards across the updated workflows. No functional impact.
27-27: Checkout action upgraded to v6. 🫧This upgrade is part of the coordinated workflow modernization across the repository. The v6 version improves credential handling by storing them under
$RUNNER_TEMPinstead of local git config.Based on external tools context.
.github/renovate.json (1)
17-49: Configuration formatting improvements with future-proofed rules. 🫧The multi-line array formatting improves readability, and the new disabled rules for
sinonandmonaco-editorare sensible preparation for selective dependency management. Since both new rules haveenabled: false, there's no immediate behavioral change.examples/package.json (1)
18-46: Dependency version bumps look comprehensive.The coordinated updates across Babel, Rollup plugins, TypeScript types, and React ecosystem packages demonstrate thorough dependency maintenance. The version increments are all minor or patch-level updates aside from the React ecosystem upgrade, which is addressed separately.
.github/workflows/ci.yml (2)
12-12: Good addition for CI resource efficiency. 🫧Adding
cancel-in-progress: trueensures that outdated workflow runs are cancelled when new commits are pushed, saving runner resources and speeding up feedback.
24-24: Checkout action consistently upgraded to v6 across all jobs.The coordinated upgrade of the checkout action to v6 across all CI jobs maintains consistency and benefits from improved credential handling.
Based on external tools context.
Also applies to: 47-47, 67-67, 93-93, 116-116, 136-136
.github/workflows/publish.yml (3)
15-17: Excellent security enhancement with npm provenance. 🫧The addition of the
permissionsblock withid-token: writeand the--provenanceflag on the npm publish command enables supply chain attestation. This allows consumers to verify the package's build provenance, aligning with modern security best practices.Also applies to: 53-53
20-27: Publish workflow modernized with latest actions and Node 24.x.The upgrades to
checkout@v6andsetup-node@v6, along with the Node 24.x version bump, keep the publish workflow aligned with the CI workflow updates. The step name correctly reflects the Node version (unlike an inconsistency found in ci.yml).
59-62: Artifact action upgraded to v6.The
upload-artifactaction upgrade to v6 is consistent with the broader workflow modernization efforts and includes security patches.Based on external tools context.
.github/workflows/upload.yml (1)
17-17: No action needed—the versions are correct. 🫧Both actions are using their latest stable releases:
download-artifact@v7is the current version, andupload-artifact@v6is the current version. The version numbers differ naturally because each action follows its own release cycle. This is not a mismatch or error.examples/src/app/components/Example.mjs (2)
5-5: Correct migration to React Router v6+ hooks! 🫧The import change from
withRoutertouseParamsaligns perfectly with React Router v6's hook-based API. This is the recommended approach for accessing route parameters.
424-424: Clever export aliasing maintains backward compatibility! 🫧Exporting
ExampleWithRouterasExampleis a smart pattern that preserves the public API while seamlessly integrating the new router hooks. External consumers remain unaffected by the internal migration.examples/src/app/components/MainLayout.mjs (1)
3-3: Spot-on React Router v6 import migration! 🫧The imports correctly reflect the v6 API changes:
RoutesreplacesSwitch, andNavigatereplacesRedirect. This aligns perfectly with the breaking changes documented in React Router v6.examples/src/app/components/Sidebar.mjs (4)
4-4: Perfect React Router v6 hook adoption! 🫧Adding
useLocationto the imports is the correct approach for accessing routing state in React Router v6. This hook provides the location object with pathname, hash, and other routing information.
15-15: Well-typed location prop! 🫧The
Propstype correctly defines the location structure withpathnameandhashproperties, matching the object shape returned byuseLocation(). This provides proper type safety for the router-provided data.
205-205: Excellent migration to pathname-based selection! 🫧The selection logic correctly switches from hash-based comparison to pathname-based comparison using
this.props.location.pathname. This aligns with React Router v6 best practices and works seamlessly with theHashRouterstill in use.The
pathname === pathcomparison at line 227 will correctly highlight the active example in the sidebar.Also applies to: 227-227
301-310: Elegant wrapper pattern with clean typing! 🫧The
SideBarWithRouterwrapper follows the same excellent pattern used inExample.mjs, usinguseLocation()to inject router state into the class component. Notably, this implementation avoids TypeScript suppressions, demonstrating proper type alignment between the hook return and the component props.The export aliasing maintains backward compatibility while integrating the new router hooks seamlessly.
examples/rollup.config.mjs (1)
264-274: Superbly handled warning suppression for React Server Components! 🫧The custom
onwarnhandler elegantly filters out React Server Components"use client"directive warnings from react-router v7+ while preserving all other build warnings. The dual check—verifying both the warning code (MODULE_LEVEL_DIRECTIVE) and message content ("use client")—ensures surgical precision and prevents accidental suppression of unrelated warnings.The comment provides excellent maintainability guidance, clearly documenting the rationale and future removal conditions.
| - name: Setup Node.js 22.x | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: 24.x | ||
| cache: "npm" |
There was a problem hiding this comment.
Step name doesn't match the Node version being used. 🫧
The step is named "Setup Node.js 22.x" but configures node-version: 24.x. This inconsistency appears throughout the workflow (lines 49-53, 69-73, 95-99, 118-122, 138-142). Please update all step names to reference "24.x" for clarity.
🔎 Suggested fix for consistent naming
- - name: Setup Node.js 22.x
+ - name: Setup Node.js 24.x
uses: actions/setup-node@v6
with:
node-version: 24.x
cache: "npm"Apply this pattern to all five occurrences in the workflow.
📝 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.
| - name: Setup Node.js 22.x | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: 24.x | |
| cache: "npm" | |
| - name: Setup Node.js 24.x | |
| uses: actions/setup-node@v6 | |
| with: | |
| node-version: 24.x | |
| cache: "npm" |
🤖 Prompt for AI Agents
.github/workflows/ci.yml around lines 26-30 (and also update the other
occurrences at 49-53, 69-73, 95-99, 118-122, 138-142): the GitHub Actions step
names read "Setup Node.js 22.x" but the configuration uses node-version: 24.x;
rename each step to "Setup Node.js 24.x" so the step name matches the configured
Node version, updating all five occurrences for consistency.
| import { spawn, execSync } from 'node:child_process'; | ||
|
|
||
| import puppeteer from 'puppeteer'; | ||
| import { launch } from 'puppeteer'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check puppeteer version and verify named export availability
# Find and display package.json files
find . -name "package.json" -path "*/examples/*" -o -name "package.json" -path "*" | head -5
# Check the installed puppeteer version in examples directory
if [ -f "examples/package.json" ]; then
echo "=== Puppeteer version in examples/package.json ==="
cat examples/package.json | grep -A 2 -B 2 "puppeteer"
elif [ -f "package.json" ]; then
echo "=== Puppeteer version in root package.json ==="
cat package.json | grep -A 2 -B 2 "puppeteer"
fi
# Search for any other puppeteer imports in the codebase
echo -e "\n=== Other puppeteer imports ==="
rg -n "from ['\"]puppeteer['\"]" --type=js --type=mjsRepository: JaclynCodes/engine_core
Length of output: 400
🌐 Web query:
puppeteer named export launch available version
💡 Result:
Short answer: There isn't a runtime named export "launch" in Puppeteer — you must call launch from the default export (e.g. import puppeteer from "puppeteer"; await puppeteer.launch()). Type definitions sometimes declare a named launch, but that is a typing artifact and not a real named export at runtime. [1][2]
Sources:
- Stack Overflow: examples and explanation about default vs named import for puppeteer. [1]
- Type definition gist showing typed
export function launch(...)(types differ from runtime). [2]
Named export launch does not exist at runtime in puppeteer 🫧
The import pattern import { launch } from 'puppeteer' will fail at runtime. Puppeteer v24.31.0 exports only a default export, not named exports. While TypeScript type definitions may declare a named launch (a typing artifact), the actual runtime module does not provide this export.
This requires reverting to: import puppeteer from 'puppeteer' and updating the corresponding usage from launch(options) to puppeteer.launch(options). The JSDoc change using Parameters<typeof launch>[0] should also be adjusted accordingly.
Also applies to: 63-63, 68-68
🤖 Prompt for AI Agents
In examples/scripts/build-thumbnails.mjs around lines 7, 63 and 68, the code
imports a non-existent named export "launch" from puppeteer and uses it;
puppeteer v24.x only provides a default export at runtime. Change the import to
the default (import puppeteer from 'puppeteer') and update usages from
launch(options) to puppeteer.launch(options); also update the JSDoc type
expression from Parameters<typeof launch>[0] to Parameters<typeof
puppeteer.launch>[0] (or equivalent) so runtime and types align.
| componentDidMount() { | ||
| const { canvas } = this; | ||
| canvas.addEventListener('mousemove', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('mousedown', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('touchmove', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('touchstart', this.mouseEvent.bind(this)); | ||
|
|
||
| // @ts-ignore engine-tsd | ||
| const dim = `${window.top.controlPanel.offsetWidth}px`; | ||
| canvas.setAttribute('style', `width: ${dim}; height: ${dim};`); | ||
| canvas.setAttribute('width', dim); | ||
| canvas.setAttribute('height', dim); | ||
| this.drawPosition(); | ||
| } | ||
|
|
||
| componentDidMount() { | ||
| const { canvas, app } = this; | ||
| // console.log("componentDidMount()", { canvas, app }); | ||
| canvas.addEventListener('mousemove', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('mousedown', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('touchmove', this.mouseEvent.bind(this)); | ||
| canvas.addEventListener('touchstart', this.mouseEvent.bind(this)); | ||
| if (!app) { | ||
| console.warn('no app'); | ||
| return; | ||
| } | ||
| this.onAppStart(); | ||
| observer.on('*:set', this.drawPosition.bind(this)); | ||
| } |
There was a problem hiding this comment.
Missing componentWillUnmount — event listeners and observer subscription leak 🚰
The class component adds event listeners and subscribes to observer events in componentDidMount, but there's no componentWillUnmount to clean them up. This can cause memory leaks and stale callbacks.
🔎 Suggested fix — add cleanup lifecycle method
componentDidMount() {
const { canvas } = this;
+ this.boundMouseEvent = this.mouseEvent.bind(this);
+ this.boundDrawPosition = this.drawPosition.bind(this);
- canvas.addEventListener('mousemove', this.mouseEvent.bind(this));
- canvas.addEventListener('mousedown', this.mouseEvent.bind(this));
- canvas.addEventListener('touchmove', this.mouseEvent.bind(this));
- canvas.addEventListener('touchstart', this.mouseEvent.bind(this));
+ canvas.addEventListener('mousemove', this.boundMouseEvent);
+ canvas.addEventListener('mousedown', this.boundMouseEvent);
+ canvas.addEventListener('touchmove', this.boundMouseEvent);
+ canvas.addEventListener('touchstart', this.boundMouseEvent);
// @ts-ignore engine-tsd
const dim = `${window.top.controlPanel.offsetWidth}px`;
canvas.setAttribute('style', `width: ${dim}; height: ${dim};`);
canvas.setAttribute('width', dim);
canvas.setAttribute('height', dim);
- observer.on('*:set', this.drawPosition.bind(this));
+ observer.on('*:set', this.boundDrawPosition);
+ this.boundDrawPosition();
}
+ componentWillUnmount() {
+ const { canvas } = this;
+ canvas.removeEventListener('mousemove', this.boundMouseEvent);
+ canvas.removeEventListener('mousedown', this.boundMouseEvent);
+ canvas.removeEventListener('touchmove', this.boundMouseEvent);
+ canvas.removeEventListener('touchstart', this.boundMouseEvent);
+ observer.off('*:set', this.boundDrawPosition);
+ }
render() {📝 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.
| componentDidMount() { | |
| const { canvas } = this; | |
| canvas.addEventListener('mousemove', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('mousedown', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('touchmove', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('touchstart', this.mouseEvent.bind(this)); | |
| // @ts-ignore engine-tsd | |
| const dim = `${window.top.controlPanel.offsetWidth}px`; | |
| canvas.setAttribute('style', `width: ${dim}; height: ${dim};`); | |
| canvas.setAttribute('width', dim); | |
| canvas.setAttribute('height', dim); | |
| this.drawPosition(); | |
| } | |
| componentDidMount() { | |
| const { canvas, app } = this; | |
| // console.log("componentDidMount()", { canvas, app }); | |
| canvas.addEventListener('mousemove', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('mousedown', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('touchmove', this.mouseEvent.bind(this)); | |
| canvas.addEventListener('touchstart', this.mouseEvent.bind(this)); | |
| if (!app) { | |
| console.warn('no app'); | |
| return; | |
| } | |
| this.onAppStart(); | |
| observer.on('*:set', this.drawPosition.bind(this)); | |
| } | |
| componentDidMount() { | |
| const { canvas } = this; | |
| this.boundMouseEvent = this.mouseEvent.bind(this); | |
| this.boundDrawPosition = this.drawPosition.bind(this); | |
| canvas.addEventListener('mousemove', this.boundMouseEvent); | |
| canvas.addEventListener('mousedown', this.boundMouseEvent); | |
| canvas.addEventListener('touchmove', this.boundMouseEvent); | |
| canvas.addEventListener('touchstart', this.boundMouseEvent); | |
| // @ts-ignore engine-tsd | |
| const dim = `${window.top.controlPanel.offsetWidth}px`; | |
| canvas.setAttribute('style', `width: ${dim}; height: ${dim};`); | |
| canvas.setAttribute('width', dim); | |
| canvas.setAttribute('height', dim); | |
| observer.on('*:set', this.boundDrawPosition); | |
| this.boundDrawPosition(); | |
| } | |
| componentWillUnmount() { | |
| const { canvas } = this; | |
| canvas.removeEventListener('mousemove', this.boundMouseEvent); | |
| canvas.removeEventListener('mousedown', this.boundMouseEvent); | |
| canvas.removeEventListener('touchmove', this.boundMouseEvent); | |
| canvas.removeEventListener('touchstart', this.boundMouseEvent); | |
| observer.off('*:set', this.boundDrawPosition); | |
| } |
| observer.on('*:set', this.drawPosition.bind(this)); | ||
| } |
There was a problem hiding this comment.
Missing initial draw call
The functional component in blend-trees-2d-directional.controls.mjs calls drawPosition() at line 71 for the initial render. This class component only subscribes to future changes but never triggers an initial draw.
🔎 Suggested fix
observer.on('*:set', this.drawPosition.bind(this));
+ this.drawPosition();
}📝 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.
| observer.on('*:set', this.drawPosition.bind(this)); | |
| } | |
| observer.on('*:set', this.drawPosition.bind(this)); | |
| this.drawPosition(); | |
| } |
🤖 Prompt for AI Agents
In examples/src/examples/animation/blend-trees-2d-cartesian.controls.mjs around
lines 116-117 the class subscribes to changes with observer.on('*:set',
this.drawPosition.bind(this)) but never triggers an initial render; add a
one-time initial call to this.drawPosition() (using the same bound context if
needed) immediately after the subscription so the control is drawn on mount
before any changes occur.
| observer.on('*:set', drawPosition); | ||
|
|
||
| const mouseEvent = (e) => { | ||
| if (e.buttons) { | ||
| position = new pc.Vec2(e.offsetX, e.offsetY).mulScalar(1 / (width / 2)).sub(pc.Vec2.ONE); | ||
| position.y *= -1.0; | ||
| modelEntity.anim.setFloat('posX', position.x); | ||
| modelEntity.anim.setFloat('posY', position.y); | ||
| drawPosition(ctx); | ||
| const rect = canvas.getBoundingClientRect(); | ||
| const x = ((e.clientX - rect.left) / (width / 2)) - 1; | ||
| const y = -(((e.clientY - rect.top) / (height / 2)) - 1); | ||
| observer.set('data.pos', { x, y }); | ||
| } | ||
| }; | ||
|
|
||
| canvas.addEventListener('mousemove', mouseEvent); | ||
| canvas.addEventListener('mousedown', mouseEvent); | ||
| }); | ||
|
|
||
| return () => { | ||
| canvas.removeEventListener('mousemove', mouseEvent); | ||
| canvas.removeEventListener('mousedown', mouseEvent); | ||
| }; | ||
| }, [observer]); |
There was a problem hiding this comment.
Missing cleanup for observer subscription 🧹
The useEffect cleanup (lines 87-90) removes canvas event listeners but doesn't unsubscribe from observer.on('*:set', drawPosition). This could lead to stale callbacks if the component remounts.
🔎 Suggested fix
canvas.addEventListener('mousemove', mouseEvent);
canvas.addEventListener('mousedown', mouseEvent);
return () => {
canvas.removeEventListener('mousemove', mouseEvent);
canvas.removeEventListener('mousedown', mouseEvent);
+ observer.off('*:set', drawPosition);
};
}, [observer]);Verify that the observer object has an off method for unsubscribing:
#!/bin/bash
# Search for observer off/unbind method definition
rg -n "\.off\s*\(" examples/src --type js --type mjs -g "*.mjs" | head -20
ast-grep --pattern 'off($_, $_) { $$$ }'🤖 Prompt for AI Agents
In examples/src/examples/animation/blend-trees-2d-directional.controls.mjs
around lines 73 to 91, the useEffect registers an observer subscription
observer.on('*:set', drawPosition) but the cleanup only removes canvas event
listeners, leaving the observer callback attached; to fix, verify the observer
exposes an off (or unsubscribe) method and call observer.off('*:set',
drawPosition) (or the correct unsubscribe API) inside the returned cleanup
function alongside removing the canvas listeners so the drawPosition handler is
properly unsubscribed when the component unmounts or observer changes.
| glslangUrl: `${rootPath}/static/lib/glslang/glslang.js`, | ||
| twgslUrl: `${rootPath}/static/lib/twgsl/twgsl.js`, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove deprecated shader URLs to align with PR objectives. 🫧
These glslangUrl and twgslUrl properties are being systematically removed across 20+ examples in this PR. Since this is a new file, it shouldn't introduce deprecated patterns that are actively being removed elsewhere.
🔎 Proposed fix
const gfxOptions = {
deviceTypes: [deviceType],
- glslangUrl: `${rootPath}/static/lib/glslang/glslang.js`,
- twgslUrl: `${rootPath}/static/lib/twgsl/twgsl.js`,
-
// disable antialiasing as gaussian splats do not benefit from it and it's expensive
antialias: false
};Based on PR summary and external tools context.
📝 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.
| glslangUrl: `${rootPath}/static/lib/glslang/glslang.js`, | |
| twgslUrl: `${rootPath}/static/lib/twgsl/twgsl.js`, | |
| const gfxOptions = { | |
| deviceTypes: [deviceType], | |
| // disable antialiasing as gaussian splats do not benefit from it and it's expensive | |
| antialias: false | |
| }; |
🤖 Prompt for AI Agents
In examples/src/examples/gaussian-splatting/flipbook.example.mjs around lines 14
to 15, the example is adding deprecated glslangUrl and twgslUrl properties;
remove these two properties from the exported configuration so the file no
longer introduces the deprecated shader URLs, and adjust any trailing commas or
object formatting as needed to keep valid JS.
|
copilot/checkout-pull-request-8 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JaclynCodes. * #8 (comment) The following files were modified: * `examples/src/app/components/Sidebar.mjs`
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Misfit <218383634+JaclynCodes@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
examples/src/examples/gaussian-splatting/crop.example.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/src/examples/gaussian-splatting/crop.example.mjs (1)
src/framework/app-base.js (1)
dt(2069-2069)
🔇 Additional comments (7)
examples/src/examples/gaussian-splatting/crop.example.mjs (7)
1-6: LGTM! Clean imports and setup. 🫧The dynamic import pattern for the shader effect is well-suited for this modular example structure.
8-47: Excellent initialization with proper cleanup! 🫧The disabled antialiasing for gaussian splats is a smart optimization, and the pixel ratio capping prevents performance issues on high-DPI displays. The resize listener cleanup on destroy is a nice touch.
49-93: Solid entity setup with defensive null checks. 🫧The optional chaining on
hotel.script?.createand subsequentcropScriptchecks demonstrate careful defensive coding. The data store integration is clean and purposeful.
130-160: Camera and post-processing configured beautifully! 🫧The subtle bloom intensity (0.03) paired with ACES tone mapping creates an elegant visual presentation for gaussian splats. The orbit camera configuration with mouse and touch support ensures smooth cross-platform interaction.
174-192: ✅ Memory leak fixed! Event listeners are now properly cleaned up. 🫧The cleanup handler on lines 183-192 correctly removes all event listeners on app destroy, addressing the previous review concern. The auto-rotate interaction detection is well-implemented with appropriate user events.
194-223: Smooth animation with elegant sine-based easing! 🫧The AABB animation respects the pause state and uses soft bounce easing for visual appeal. The auto-rotate re-enabling logic after user inactivity creates a polished idle experience.
226-226: LGTM! 🫧Clean export of the app instance.
| const checkMaterial = () => { | ||
| const material = getMaterial(); | ||
| if (material) { | ||
| updatePreciseDefine(data.get('precise')); | ||
| } else { | ||
| setTimeout(checkMaterial, 100); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add a retry limit to prevent infinite polling. 🫧
The checkMaterial function polls indefinitely without an upper bound on retries. If the material never becomes available (e.g., due to an initialization failure), this creates a memory leak through accumulating setTimeout calls.
🔎 Proposed fix with retry limit
- // Wait for material to be available, then set initial state
- const checkMaterial = () => {
+ // Wait for material to be available, then set initial state
+ const maxRetries = 50; // 5 seconds at 100ms intervals
+ let retryCount = 0;
+ const checkMaterial = () => {
const material = getMaterial();
if (material) {
updatePreciseDefine(data.get('precise'));
- } else {
+ } else if (retryCount < maxRetries) {
+ retryCount++;
setTimeout(checkMaterial, 100);
+ } else {
+ console.warn('Failed to retrieve gsplat material after maximum retries');
}
};
checkMaterial();📝 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.
| const checkMaterial = () => { | |
| const material = getMaterial(); | |
| if (material) { | |
| updatePreciseDefine(data.get('precise')); | |
| } else { | |
| setTimeout(checkMaterial, 100); | |
| } | |
| }; | |
| const maxRetries = 50; // 5 seconds at 100ms intervals | |
| let retryCount = 0; | |
| const checkMaterial = () => { | |
| const material = getMaterial(); | |
| if (material) { | |
| updatePreciseDefine(data.get('precise')); | |
| } else if (retryCount < maxRetries) { | |
| retryCount++; | |
| setTimeout(checkMaterial, 100); | |
| } else { | |
| console.warn('Failed to retrieve gsplat material after maximum retries'); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In examples/src/examples/gaussian-splatting/crop.example.mjs around lines 115 to
122, the checkMaterial poller currently retries forever; add a retry limit and
stop scheduling timeouts after it is reached to prevent unbounded polling and
memory growth. Implement a counter (e.g., attempts) and a configurable
maxRetries, increment attempts on each scheduled retry, only call
setTimeout(checkMaterial, 100) while attempts < maxRetries, and when the limit
is hit stop polling and surface a clear warning/error (or fallback behavior) so
the caller can handle the initialization failure.
8 participants
Add CodeQL analysis workflow configuration
Chore(deps-dev): bump js-yaml from 4.1.0 to 4.1.1
Bumps js-yaml from 4.1.0 to 4.1.1.
updated-dependencies:
Bumps glob from 10.4.5 to 10.5.0.
updated-dependencies:
Bumps serialize-javascript from 6.0.1 to 6.0.2.
updated-dependencies:
Description
Brief description of what this PR does.
Fixes #
Checklist