bhouston · October 9, 2026 13:10
diff --git a/gistfile1.txt b/gistfile1.txt
 # Summary: Mugen87's review preferences


 Grouped by theme, most frequent first. Each rule cites the PRs where it came up.


 ## 1. Don't add defensive validation or throws


 This is his most repeated request.


 - Don't add `throw new Error(...)` checks for invalid input, unsupported types, or wrong configurations. "We usually don't add this kind of validation." (#34836, #34838 ×4)
 - If a restriction exists, document it in JSDoc and skip the runtime check. "You already documented the restriction in the JSDoc so this check can go as well." (#34838)
 - Don't write unit tests whose only job is to check that validation fires. He deleted these along with the throw. (#34836)
 - Exception: throwing is fine when a feature *cannot* exist without hardware support (subgroups). If the feature can be emulated, emulate it instead of throwing (see §5). (#34412)


 ## 2. Keep comments short. No long explanations


 He removed long explanatory comments himself on at least 9 PRs. This is the most common edit he pushed to your branches.


 - Don't put "Regression coverage for…" blocks at the top of test files that explain the bug's history. He deleted every one: #34337, #34338, #34340, #34341, #34342. "Can we strip these verbose comments in the PRs? The more compact inline comments in the tests are sufficient."
 - In source code, cut explanations of *why* the old code was wrong down to one short line, or remove them. Examples of his rewrites:
  - A 13-line note on the GLSL int/uint constructor became `// integer types support bitcast implicitly via the type constructor` (#34430)
  - A 10-line note on QUnit's `done` race became `// Wait for the QUnit test results` (#34349, #34350)
  - An 8-line note on column-major `mat4` in `RotateNode` was deleted entirely (#34338)
  - A 10-line note on the sinc singularity was deleted (#34337)
  - A two-line comment was tightened to `// Clone so the wrapping and orientation set below don't modify the handler's texture.` (#34575)
 - House style: short sentences, often lowercase, with a blank line after the comment before the code block.


 ## 3. Minimal, compact code


 - Use fewer checks, better variable names, and less verbose code. He rewrote `getBitcastMethod()` along those lines (#34430). Prefer one general condition (`inputComponentType !== 'float' && componentType !== 'float'`) over enumerating cases.
 - Remove code your change makes obsolete, such as unused polyfills and method-table entries (#34430).
 - Remove dead lines and leftovers: an `offset +=` with no effect (#33950), a leftover assertion in a test (#34439).
 - Combine imports from the same module into one line: `import { float, gain, pcurve } from 'three/tsl';` (#34340).
 - No trailing blank line at the end of `three.addons.unit.js`-style index files (#34341).
 - Reuse existing definitions instead of duplicating them, e.g. use `TEXTURE_ADDRESS_MODE_WRAPPING` from `MaterialXDocument` rather than redefining the modes in `MaterialXUtils` (#34523).
 - Don't create helper or utility modules for logic that is really specific to one pass. "The camera and pass setup is so pass specific that helpers are not necessarily required." (#34838)


 ## 4. Keep PRs small, focused, and clean


 - Keep them small. Split large features into a foundation PR plus a feature PR: "Implement 16 bit logic first with a simple example and then when merged focus on the Neural Texture Compression components." (#34365). mrdoob said the same: "try to avoid doing big PRs like these please" (#34868).
 - Don't mix two PRs' changes. He asked you to remove the `BufferGeometry`/`Box3` changes from #34205 because they belonged to #34204.
 - Never include `build/` files (#34358).
 - Rebase promptly when he asks (#34429, #34432).
 - He hands core-architecture changes (`NodeBuilder`, `TempNode`) and design decisions (FP16) to **sunag**. Expect those PRs to wait on sunag, and follow sunag's design direction (generic `setPrecision()` rather than separate `h*` types) (#34339, #34365).


 ## 5. TSL / node-system conventions


 - **Emulate rather than throw.** If a WGSL feature can be expressed with plain integer or bit ops, add GLSL and WGSL fallbacks so it works everywhere. "We only lose hardware acceleration but that is acceptable." Use `BitcountNode` as the template (#34412).
 - **Feature detection:** check `navigator.gpu.wgslLanguageFeatures.has( '...' )`, not only `backend.isWebGPUBackend` (#34412).
 - **Placement:** he put the final `Packed4x8IntegerNode` in core `src/nodes/math/` next to `PackFloatNode`, not in `examples/jsm/tsl` (his rewrite #34451, which replaced your #34412).
 - **Guard against missing attributes** with a fallback inside an `Fn`, e.g. `builder.geometry.hasAttribute( 'tangent' )`. Without tangents, `bitangentLocal` produces NaN (#34576).
 - **Prefer math tricks over `select()` branches** for singularities: `abs( x ).max( 1e-6 ).toConst()` instead of `select( abs( x ).lessThan( eps ), 1, … )`, with the reasoning written up (#34337).
 - He is willing to accept small precision tradeoffs (e.g. losing `highPrecision` 64-bit in an ArrayCamera path) when the alternative scales badly (#34912).


 ## 6. Public API surface and loader behavior


 - Don't widen the public API with internals. "Can we please not add the raw `MaterialXDocument` to the result? It widens the public surface… carries loader internals." (#34439)
 - Match the behavior of established loaders, with `GLTFLoader` as the reference. A missing texture should be a `warning` and loading should continue, not fail the whole load (#34439).
 - Call out behavior changes. Resolving the load only after all sub-resources finish was something he had to think about first (#34523 → #34439).
 - Check texture orientation carefully. `flipY` differs between ImageBitmap and EXR (bottom-first) and has no meaning for compressed textures. He asked for a real EXR test file in the MaterialX example (#34575).


 ## 7. Architecture and dependencies


 - Don't create hard dependencies from shared addons (`GLTFLoader`, `GLTFExporter`, editor) on TSL/WebGPU. Ship that integration as opt-in extensions registered at app level (`loader.register( new GLTFGaussianSplattingExtension() )`), with the same approach for the exporter (#33950).
 - Don't hand-split or regenerate vendored libraries (`fflate`), and don't add generator scripts. Rely on tree-shaking, or fix the problem upstream (#34210).
 - He weighs the maintenance cost of large features. He asked whether Gaussian splats should live in a separate repo, and dropped the objection once shown industry adoption (#33950).
 - **WebGLRenderer `EffectComposer` passes are maintenance-only.** Small fixes are fine. No large refactors or new pass families; effects work goes into the node-based pipeline. This is why the GBufferPass series was closed (#34838, #34868, and implicitly #34842, #34869, #34875).


 ## 8. Performance vs. readability


 - He rejects selective fast paths that bypass existing abstractions (accessors, math classes) unless they are applied consistently to every comparable loop *and* the code is actually hot per frame (#34204).
 - When asked, he prefers to put the policy behind a named method on the abstraction (`BufferAttribute.supportsDirectAccess()`) rather than inline checks (#34204).
 - No `test/benchmark/` suite. It was removed on purpose; share benchmarks via jsbench or jsfiddle (#34204).


 ## 9. Tests


 - Unit tests import from `src/`, never `build/` (#34439). This became your lint rule (#34541), scoped to `test/unit/**/*.js` and named `unit test rules`, because E2E tests may use the bundle.
 - Don't add test-only code to examples (`window.TESTING`) (#34439).
 - If an E2E screenshot is flaky, he adds the example to the exception list in `test/e2e/puppeteer.js` (#33950).


 ## 10. Examples


 His cleanup PR #34185 on `webgpu_gaussian_splatting.html`, right after merge, is the best template:


 - Use `renderer.inspector = new Inspector()` and `renderer.inspector.createParameters( '…' )` instead of lil-gui `GUI`.
 - Drop the `WebGPU.isAvailable()` check and the `forceWebGL` URL/GUI toggle.
 - Use `new THREE.WebGPURenderer()` with default options (no `antialias: false` unless needed).
 - Name the animation loop callback `animate`, not `render`.
 - Lint spacing on unary minus: `Math.PI * - 1.10`, not `* -1.10`.


 From review comments:


 - Every external link needs `target="_blank" rel="noopener"` (#34207).
 - Prefer one general CSS rule (`#info a { pointer-events: auto; }`) over several specific ones (#34207).
 - Keep default assets small (under ~10 MB). Don't load a 112 MB remote asset by default (#33950).
 - Don't delete GUI or features that were added on purpose in earlier PRs (#34439).


 ## 11. Small nits he catches


 - Typos in messages ("guarrented" → "guaranteed") (#34541).
 - Precise naming and scoping of config entries (#34541).


 ## Suggested checklist before opening a PR


 1. No new `throw`, validation, or `console.error` checks. Document restrictions in JSDoc.
 2. Comments at most 1–2 lines, no bug-history essays, no regression header blocks in tests.
 3. Diff touches only what the title says, no `build/` files, rebased on `dev`.
 4. No new helper modules, public result fields, or hard dependencies unless clearly necessary.
 5. New loader behavior should match `GLTFLoader` and other existing three.js precedents. Any behavior change is called out.
 6. TSL features have GLSL and WGSL fallbacks plus feature detection, unless hardware-only.
 7. Examples: Inspector, `animate()`, default renderer options, `target="_blank" rel="noopener"`, small assets, no test hooks.
 8. Large work is split. Core node-system changes are pre-discussed with sunag.
 9. Nothing new for the legacy `EffectComposer` beyond small fixes.


 # Mugen87 follow-up PRs on `dev` that reworked your merged code


 - **#34451** WebGPURenderer: Add `packed_4x8_integer_dot_product` support. His own alternative to your #34412: `Packed4x8IntegerNode` in core `src/nodes/math/` next to `PackFloatNode`, with emulation when the extension is missing or on WebGL 2.
 - **#34185** Examples: Clean up. Cleaned up `webgpu_gaussian_splatting.html` the day after #33950 merged (see §10).
 - **#34057** MeshPhysicalMaterial: Fix diffuse Fresnel blend for `retroreflective`. Pulled your diffuse Fresnel fix out of #33995/#33999 so it could land without waiting on the IBL discussion.
 - **#34010** Examples: Clean up. Follow-up to #34001, applying mrdoob's `ro.me` → `rome.mrdoob.com` link change.
	# Summary: Mugen87's review preferences


	Grouped by theme, most frequent first. Each rule cites the PRs where it came up.


	## 1. Don't add defensive validation or throws


	This is his most repeated request.


	- Don't add `throw new Error(...)` checks for invalid input, unsupported types, or wrong configurations. "We usually don't add this kind of validation." (#34836, #34838 ×4)
	- If a restriction exists, document it in JSDoc and skip the runtime check. "You already documented the restriction in the JSDoc so this check can go as well." (#34838)
	- Don't write unit tests whose only job is to check that validation fires. He deleted these along with the throw. (#34836)
	- Exception: throwing is fine when a feature cannot exist without hardware support (subgroups). If the feature can be emulated, emulate it instead of throwing (see §5). (#34412)


	## 2. Keep comments short. No long explanations


	He removed long explanatory comments himself on at least 9 PRs. This is the most common edit he pushed to your branches.


	- Don't put "Regression coverage for…" blocks at the top of test files that explain the bug's history. He deleted every one: #34337, #34338, #34340, #34341, #34342. "Can we strip these verbose comments in the PRs? The more compact inline comments in the tests are sufficient."
	- In source code, cut explanations of why the old code was wrong down to one short line, or remove them. Examples of his rewrites:
	- A 13-line note on the GLSL int/uint constructor became `// integer types support bitcast implicitly via the type constructor` (#34430)
	- A 10-line note on QUnit's `done` race became `// Wait for the QUnit test results` (#34349, #34350)
	- An 8-line note on column-major `mat4` in `RotateNode` was deleted entirely (#34338)
	- A 10-line note on the sinc singularity was deleted (#34337)
	- A two-line comment was tightened to `// Clone so the wrapping and orientation set below don't modify the handler's texture.` (#34575)
	- House style: short sentences, often lowercase, with a blank line after the comment before the code block.


	## 3. Minimal, compact code


	- Use fewer checks, better variable names, and less verbose code. He rewrote `getBitcastMethod()` along those lines (#34430). Prefer one general condition (`inputComponentType !== 'float' && componentType !== 'float'`) over enumerating cases.
	- Remove code your change makes obsolete, such as unused polyfills and method-table entries (#34430).
	- Remove dead lines and leftovers: an `offset +=` with no effect (#33950), a leftover assertion in a test (#34439).
	- Combine imports from the same module into one line: `import { float, gain, pcurve } from 'three/tsl';` (#34340).
	- No trailing blank line at the end of `three.addons.unit.js`-style index files (#34341).
	- Reuse existing definitions instead of duplicating them, e.g. use `TEXTURE_ADDRESS_MODE_WRAPPING` from `MaterialXDocument` rather than redefining the modes in `MaterialXUtils` (#34523).
	- Don't create helper or utility modules for logic that is really specific to one pass. "The camera and pass setup is so pass specific that helpers are not necessarily required." (#34838)


	## 4. Keep PRs small, focused, and clean


	- Keep them small. Split large features into a foundation PR plus a feature PR: "Implement 16 bit logic first with a simple example and then when merged focus on the Neural Texture Compression components." (#34365). mrdoob said the same: "try to avoid doing big PRs like these please" (#34868).
	- Don't mix two PRs' changes. He asked you to remove the `BufferGeometry`/`Box3` changes from #34205 because they belonged to #34204.
	- Never include `build/` files (#34358).
	- Rebase promptly when he asks (#34429, #34432).
	- He hands core-architecture changes (`NodeBuilder`, `TempNode`) and design decisions (FP16) to sunag. Expect those PRs to wait on sunag, and follow sunag's design direction (generic `setPrecision()` rather than separate `h*` types) (#34339, #34365).


	## 5. TSL / node-system conventions


	- Emulate rather than throw. If a WGSL feature can be expressed with plain integer or bit ops, add GLSL and WGSL fallbacks so it works everywhere. "We only lose hardware acceleration but that is acceptable." Use `BitcountNode` as the template (#34412).
	- Feature detection: check `navigator.gpu.wgslLanguageFeatures.has( '...' )`, not only `backend.isWebGPUBackend` (#34412).
	- Placement: he put the final `Packed4x8IntegerNode` in core `src/nodes/math/` next to `PackFloatNode`, not in `examples/jsm/tsl` (his rewrite #34451, which replaced your #34412).
	- Guard against missing attributes with a fallback inside an `Fn`, e.g. `builder.geometry.hasAttribute( 'tangent' )`. Without tangents, `bitangentLocal` produces NaN (#34576).
	- Prefer math tricks over `select()` branches for singularities: `abs( x ).max( 1e-6 ).toConst()` instead of `select( abs( x ).lessThan( eps ), 1, … )`, with the reasoning written up (#34337).
	- He is willing to accept small precision tradeoffs (e.g. losing `highPrecision` 64-bit in an ArrayCamera path) when the alternative scales badly (#34912).


	## 6. Public API surface and loader behavior


	- Don't widen the public API with internals. "Can we please not add the raw `MaterialXDocument` to the result? It widens the public surface… carries loader internals." (#34439)
	- Match the behavior of established loaders, with `GLTFLoader` as the reference. A missing texture should be a `warning` and loading should continue, not fail the whole load (#34439).
	- Call out behavior changes. Resolving the load only after all sub-resources finish was something he had to think about first (#34523 → #34439).
	- Check texture orientation carefully. `flipY` differs between ImageBitmap and EXR (bottom-first) and has no meaning for compressed textures. He asked for a real EXR test file in the MaterialX example (#34575).


	## 7. Architecture and dependencies


	- Don't create hard dependencies from shared addons (`GLTFLoader`, `GLTFExporter`, editor) on TSL/WebGPU. Ship that integration as opt-in extensions registered at app level (`loader.register( new GLTFGaussianSplattingExtension() )`), with the same approach for the exporter (#33950).
	- Don't hand-split or regenerate vendored libraries (`fflate`), and don't add generator scripts. Rely on tree-shaking, or fix the problem upstream (#34210).
	- He weighs the maintenance cost of large features. He asked whether Gaussian splats should live in a separate repo, and dropped the objection once shown industry adoption (#33950).
	- WebGLRenderer `EffectComposer` passes are maintenance-only. Small fixes are fine. No large refactors or new pass families; effects work goes into the node-based pipeline. This is why the GBufferPass series was closed (#34838, #34868, and implicitly #34842, #34869, #34875).


	## 8. Performance vs. readability


	- He rejects selective fast paths that bypass existing abstractions (accessors, math classes) unless they are applied consistently to every comparable loop and the code is actually hot per frame (#34204).
	- When asked, he prefers to put the policy behind a named method on the abstraction (`BufferAttribute.supportsDirectAccess()`) rather than inline checks (#34204).
	- No `test/benchmark/` suite. It was removed on purpose; share benchmarks via jsbench or jsfiddle (#34204).


	## 9. Tests


	- Unit tests import from `src/`, never `build/` (#34439). This became your lint rule (#34541), scoped to `test/unit/*/.js` and named `unit test rules`, because E2E tests may use the bundle.
	- Don't add test-only code to examples (`window.TESTING`) (#34439).
	- If an E2E screenshot is flaky, he adds the example to the exception list in `test/e2e/puppeteer.js` (#33950).


	## 10. Examples


	His cleanup PR #34185 on `webgpu_gaussian_splatting.html`, right after merge, is the best template:


	- Use `renderer.inspector = new Inspector()` and `renderer.inspector.createParameters( '…' )` instead of lil-gui `GUI`.
	- Drop the `WebGPU.isAvailable()` check and the `forceWebGL` URL/GUI toggle.
	- Use `new THREE.WebGPURenderer()` with default options (no `antialias: false` unless needed).
	- Name the animation loop callback `animate`, not `render`.
	- Lint spacing on unary minus: `Math.PI * - 1.10`, not `* -1.10`.


	From review comments:


	- Every external link needs `target="_blank" rel="noopener"` (#34207).
	- Prefer one general CSS rule (`#info a { pointer-events: auto; }`) over several specific ones (#34207).
	- Keep default assets small (under ~10 MB). Don't load a 112 MB remote asset by default (#33950).
	- Don't delete GUI or features that were added on purpose in earlier PRs (#34439).


	## 11. Small nits he catches


	- Typos in messages ("guarrented" → "guaranteed") (#34541).
	- Precise naming and scoping of config entries (#34541).


	## Suggested checklist before opening a PR


	1. No new `throw`, validation, or `console.error` checks. Document restrictions in JSDoc.
	2. Comments at most 1–2 lines, no bug-history essays, no regression header blocks in tests.
	3. Diff touches only what the title says, no `build/` files, rebased on `dev`.
	4. No new helper modules, public result fields, or hard dependencies unless clearly necessary.
	5. New loader behavior should match `GLTFLoader` and other existing three.js precedents. Any behavior change is called out.
	6. TSL features have GLSL and WGSL fallbacks plus feature detection, unless hardware-only.
	7. Examples: Inspector, `animate()`, default renderer options, `target="_blank" rel="noopener"`, small assets, no test hooks.
	8. Large work is split. Core node-system changes are pre-discussed with sunag.
	9. Nothing new for the legacy `EffectComposer` beyond small fixes.


	# Mugen87 follow-up PRs on `dev` that reworked your merged code


	- #34451 WebGPURenderer: Add `packed_4x8_integer_dot_product` support. His own alternative to your #34412: `Packed4x8IntegerNode` in core `src/nodes/math/` next to `PackFloatNode`, with emulation when the extension is missing or on WebGL 2.
	- #34185 Examples: Clean up. Cleaned up `webgpu_gaussian_splatting.html` the day after #33950 merged (see §10).
	- #34057 MeshPhysicalMaterial: Fix diffuse Fresnel blend for `retroreflective`. Pulled your diffuse Fresnel fix out of #33995/#33999 so it could land without waiting on the IBL discussion.
	- #34010 Examples: Clean up. Follow-up to #34001, applying mrdoob's `ro.me` → `rome.mrdoob.com` link change.
No results found