Claude Opus dev/core/Animations/ Review#18234
Conversation
- Fix inverted toString separator logic in Animation (comma on first item instead of subsequent) - Fix missing .length in AnimationGroup.toString (printed array object instead of count) - Guard against division by zero in AnimatorAvatar proportionRatio calculation - Fix vertical axis auto-detection comparing only 2 of 3 axes in AnimatorAvatar - Throw Error instead of string literal in PathCursor.move - Remove dead duplicated RegisterTargetForLateAnimationBinding from animatable.core.ts - Change == to === in ElasticEase.easeInCore for strict equality consistency - Rename shadowed loop variable in AnimatorAvatar._fixAnimationGroup - Replace verbose manual iterator patterns with for-of loops in AnimatorAvatar - Add audit summary document Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The project's tsconfig does not enable --downlevelIteration, so for-of on Set<T> causes TS2802. Restored the original manual iterator pattern (.keys() / .next() / .done). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Switching back to for-of to identify which build command requires the manual iterator pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The UMD build (packages/public/umd/babylonjs/tsconfig.build.json) targets ES5, which doesn't support for-of on Set without --downlevelIteration. The manual iterator pattern is required. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents that the verbose iterator pattern is required because the UMD build targets ES5 and for-of on Set needs --downlevelIteration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18234/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18234/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18234/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
Thank you for taking the time to put this PR together — we appreciate the effort and the interest in improving the Babylon.js codebase!
That said, we'd like to be transparent: while the individual bug findings here are mostly legitimate, we don't think external AI-driven code audits are the best path forward for the project. This is something we can (and already do) ourselves for parts of the codebase, using AI tooling that is specifically configured for our codebase conventions, internal context, and workflow. Microsoft also has direct partnerships with AI providers (OpenAI, Anthropic, etc.) that give us access to tailored models and tooling better suited to this kind of work. We plan to continue doing more of these AI-assisted reviews across the codebase in the coming weeks and months.
We'd rather focus review bandwidth on PRs that address specific issues, implement features, or fix bugs that contributors have encountered directly.
Regarding the specific changes in this PR, here are our findings:
Issues to address
1. RegisterTargetForLateAnimationBinding must not be removed
This function is exported via the barrel chain (animatable.core.ts → animatable.ts → index.ts → package root), making it a public API. Removing it is a breaking change. It should be kept as-is.
2. @internal JSDoc accidentally removed from ElasticEase.easeInCore
The diff replaces the /** @internal */ JSDoc with a plain block comment, removing the @internal annotation. Compare with BackEase.easeInCore where it was correctly preserved.
3. claude/ directory should not be committed
Audit summaries should live in the PR description, not as committed files in the repository.
4. Inline "Claude explanation" comments should be removed
Every change includes a multi-line /* Feel free to delete this comment... */ block. These are not appropriate for production code — explanations belong in the commit message or PR description.
Correct fixes ✅
The following are legitimate bugs that are correctly fixed:
animation.ts— toString separator logic was inverted.animationGroup.ts— Missing.lengthon array concatenation.animatorAvatar.ts— Vertical axis auto-detection comparison was flawed; division by zero was unguarded; inner loop variable shadowed outeri.easing.ts—==→===for strict equality.pathCursor.ts—throw "string"→throw new Error(...)for proper stack traces.
…tion artifacts - Restore RegisterTargetForLateAnimationBinding as exported public API - Fix @internal JSDoc on ElasticEase.easeInCore (was accidentally removed) - Remove claude/ directory from tracked files - Remove all inline "Claude explanation" comments from production code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep only the i→j rename for the inner loop variable that shadowed the outer loop's i. Revert the vertical axis auto-detection and division-by-zero guard changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thank you so much, @Popov72, for your review, and I love what you and the team are doing for Babylon.js 😄 The PR now only includes the 5 correct fixes you shared in your reply. |
|
I'm not sure why you removed the fix for the vertical axis auto-detection code? This is a real bug :) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
Ooh my mistake on removing that fix! I misread that only the shadowed |
Please see @Popov72 's comment here: #18234 (comment) I accidentally removed some of the fixes from Claude, and this PR looks to restore them. Thank you for your help! --------- Co-authored-by: gbz <gbz@gbzs-MacBook-Pro.local> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: local <local@local.local>
4 participants
I'm experimenting with using Claude Opus to analyze the Babylon.js source. To make the context manageable for Claude, I'm just doing 1 folder at a time (
dev/core/Animations/in this case).Please let me know if these kinds of analyses are not valuable. I'm sure that not all of the changes are done in the way the Babylon.js Core Team deems best, so please feel free to edit/discard any of them.
My motivation for doing this is Claude did an amazing job analyzing my projects. And since Babylon.js is the most important framework I depend on, I thought that Claude's input here could hugely benefit my projects and others.
If you do find this valuable, please let me know if there are particular folders you'd like to go through. Otherwise, I'd like to go through commonly used folders like
Bones/,Cameras/,Engines/, and more.I verified that the following were able to run with the changes: