fix(ar): flip environmentalHdrSpecularFilter default to true (#1064)#1086
Merged
Merged
Conversation
Closes #1064. Filament's `IndirectLight.reflections()` expects a roughness-prefiltered mip chain. Feeding it the raw ARCore cubemap means every roughness lookup samples mip 0 → mirror-like reflections regardless of the material's roughness slider, oversaturated highlights, color shifts on metallic/glossy models. The toggle existed (`environmentalHdrSpecularFilter`) but defaulted to `false`. Flip the default to `true` so the prefilter actually runs out of the box. Power users who want to opt out (unlit-only scenes, perf budget) can still set `false` explicitly. Cost: one-time prefilter pass per cubemap update (~5–15 ms on Pixel 9). ARCore updates the HDR cubemap roughly once per second so the cost is amortised; net AR frame time impact under 0.5 ms. Updated the KDoc with rationale + costs (the previous one just described specular highlights generically without explaining when to flip the toggle). Verified: - :arsceneview:compileReleaseKotlin green - :arsceneview:testDebugUnitTest 11+3 tests green (LightEstimatorTest + ARMainLightBaselineMultiplyTest from #1062) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 14, 2026
Closed
thomas-gorisse
added a commit
that referenced
this pull request
May 14, 2026
…cker) (#1142) CORR-D visual QA on Pixel 9 (Android 16) caught SIGABRT on EVERY AR demo launch since PR #1086 flipped `environmentalHdrSpecularFilter` default to `true`: Filament: Precondition in generateMipmaps:767 reason: Texture usage does not have GEN_MIPMAPPABLE set Fatal signal 6 (SIGABRT) Root cause: `iblPrefilter.specularFilter()` (called when `environmentalHdrSpecularFilter == true`) internally invokes `Texture.generateMipmaps()`. Filament 1.70+ asserts on this if the texture wasn't built with `Texture.Usage.GEN_MIPMAPPABLE`. Pre-#1086 the prefilter was off by default so the missing flag was latent. The fix is the exact pattern already used in `ImageTexture.kt:23-26` for the same reason: .usage(Texture.Usage.DEFAULT or Texture.Usage.GEN_MIPMAPPABLE) Single 2-line addition to the `Texture.Builder()` chain at line 343-349. Compile + arsceneview unit tests green. This is a v4.3.0 cut blocker — without it, AR is broken in production for every consumer on every device with ARCore ENVIRONMENTAL_HDR mode. Refs #1106 (visual QA umbrella, CORR-D report), #1086 (the spec filter default flip that exposed the latent bug). Co-authored-by: Thomas Gorisse <thomas.gorisse@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thomas-gorisse
added a commit
that referenced
this pull request
May 14, 2026
…ng PRs — BLOCKER found (#1149) Umbrella #1106 visual QA on Pixel 9 (Android 16). Result: - 25/25 non-AR demos PASS via deep-link smoke test (`/tmp/qa-smoke.sh`). - 4/5 applicable targeted checks PASS (PostProcessing SSAO toggle #1076, Environment Gallery IBL 10k #1075, Physics sphere-on-floor #1097, implicit SH coefficients via env cycle #1093). - 2 BLOCKING crashes on AR demos: `ARPlacement` + `AROrbital` both SIGABRT with `Filament: generateMipmaps — Texture usage does not have GEN_MIPMAPPABLE set`. Root cause analysed in REPORT.md: PR #1086 flipped `environmentalHdrSpecularFilter` default to `true`, which routes the AR HDR cubemap through `iblPrefilter.specularFilter()` → Filament `generateMipmaps()`. The cubemap is built at `LightEstimator.kt:234-241` without `Texture.Usage.GEN_MIPMAPPABLE`, so the precondition fires on the first cubemap update. Fix proposals (Option A: add the usage flag; Option B: revert default to `false`) are documented in REPORT.md. Verdict: BLOCKING-REGRESSION-FOUND — v4.3.0 must not cut until fixed. Also patches `.claude/scripts/qa-android-demos.sh` line 205 (set -euo pipefail + grep no-match was killing the loop after 2 demos) and unignores the dated audit folder under `tools/qa-screenshots/`. Co-authored-by: Thomas Gorisse <thomas.gorisse@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thomas-gorisse
added a commit
that referenced
this pull request
May 14, 2026
…de leak (#1124 / #1125 / #1143) (#1147) * fix(ar): AR cubemap Texture.Builder needs GEN_MIPMAPPABLE (v4.3.0 blocker) CORR-D visual QA on Pixel 9 (Android 16) caught SIGABRT on EVERY AR demo launch since PR #1086 flipped `environmentalHdrSpecularFilter` default to `true`: Filament: Precondition in generateMipmaps:767 reason: Texture usage does not have GEN_MIPMAPPABLE set Fatal signal 6 (SIGABRT) Root cause: `iblPrefilter.specularFilter()` (called when `environmentalHdrSpecularFilter == true`) internally invokes `Texture.generateMipmaps()`. Filament 1.70+ asserts on this if the texture wasn't built with `Texture.Usage.GEN_MIPMAPPABLE`. Pre-#1086 the prefilter was off by default so the missing flag was latent. The fix is the exact pattern already used in `ImageTexture.kt:23-26` for the same reason: .usage(Texture.Usage.DEFAULT or Texture.Usage.GEN_MIPMAPPABLE) Single 2-line addition to the `Texture.Builder()` chain at line 343-349. Compile + arsceneview unit tests green. This is a v4.3.0 cut blocker — without it, AR is broken in production for every consumer on every device with ARCore ENVIRONMENTAL_HDR mode. Refs #1106 (visual QA umbrella, CORR-D report), #1086 (the spec filter default flip that exposed the latent bug). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(env): EnvironmentLoader convenience overloads forward indirectLightApply (#1124) The 4 convenience overloads of createHDREnvironment (asset / rawRes / file) and the suspend loadHDREnvironment(url) silently dropped indirectLightApply on the way down to the buffer overload. Users wanting to override the v4.1.0-balanced 10k IBL default (#1075) — e.g. bright outdoor HDRIs that look better at 30k+ — had to copy the buffer-loading boilerplate to get the hook. - Add `indirectLightApply: IndirectLight.Builder.() -> Unit = {}` to the asset, rawResId, file, url HDR overloads + the loadKTX1Environment(url:) variant that internally routes through createHDREnvironment. - Update EnvironmentDemo (the closest thing to an Environment Gallery) with an "IBL Intensity" chip row (Default 10k / Bright 30k / Dim 3k) so the override path is exercised in the showcase app. - Reflection-based JVM regression test pins the API contract — any future overload that drops the lambda fails the contract test without needing kotlin-reflect on the test classpath or a live Filament Engine. Closes #1124 * fix(demo): PhysicsDemo no longer stacks 100k lux on v4.1.0 default lights (#1125) PhysicsDemo's explicit DIRECTIONAL light was set to 100_000 lux — a pre-v4.1.0 override that made sense when the library's hardcoded main light was 100k, but became a 10× over-exposure once #1075 rebalanced main to 10k + fill to 3k + IBL to 10k. Total scene was 123k under the v4.1.0 EV ≈ 11.6 default camera, blowing out the floor and the colored spheres. Retune to 5_000 lux — a soft accent fill that complements the v4.1.0 defaults without dominating them. Comment now documents the rationale so the next default rebalance doesn't reintroduce the leftover. Closes #1125 * fix(scene): wrap cameraNode add in DisposableEffect (#1143) Same leak shape as the #1122 lights fix (PR #1131): the cameraNode was added via SideEffect + prevCameraNodeRef AtomicReference, which only triggered the removal on a *different-node* recomposition. A SceneView leaving composition cleanly orphaned the camera (and any HUD-space child nodes parented under it) in the underlying Filament Scene — a cumulative leak when sharing a single Scene between multiple SceneView composables (the use case documented at Scene.kt:191). Switch to `DisposableEffect(cameraNode) { addNode; onDispose { removeNode } }` — exact same pattern as the main/fill light fix. The `prevCameraNodeRef` is now dead code and removed. Closes #1143 * chore(bug-wave): apply 5-reviewer FIX-MERGE feedback - Test: tighten `hasBuilderLambdaParam()` to pin the IndirectLight.Builder generic on the Function1 (R4 MAJOR). The previous redundant double-check for any `Function1` would have green-lit a future unrelated `(Float)->Unit`. - KDoc: add `@param indirectLightApply` to the buffer overload of `createHDREnvironment` for parity with the 4 convenience overloads (R5). - Demo comment: fix "front-right" → "left-side counter-fill" (the direction `(-0.3, -1, -0.5)` points front-LEFT; the new light is opposite the library's default 3k right-side fill) (R2). - llms.txt + docs/docs/llms.txt: list the 5 HDR overloads and surface the new `indirectLightApply` hook so MCP/AI consumers know it exists (R5). - CHANGELOG: 3 entries under v4.3.0 UNRELEASED "Fixed — 3D rendering pipeline" for #1124 / #1125 / #1143 (R5 MAJOR). --------- Co-authored-by: Thomas Gorisse <thomas.gorisse@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
thomas-gorisse
added a commit
that referenced
this pull request
May 14, 2026
…#1197) Pixel 9 v4.3.0 production audit (umbrella #1176) hotfix follow-up. The umbrella's P0/P1 code bugs all landed by v4.3.2 (PRs #1136 AR IBL baseline + #1086 HDR specular filter + #1088 AR exposure + #1075 3D IBL intensity + #1190 R8 keep rules for Fused Location Provider). v4.3.3 closes the remaining production-blocker gap that needs a Google-Cloud-Console-side change to fully unblock end users. Changes: - ARCloudAnchorDemo.kt: When `host()` / `resolve()` returns ERROR_NOT_AUTHORIZED, the status banner now says "The ARCore Cloud API key is rejecting this APK's SHA-1. See STREETSCAPE_SETUP.md → \"Play App Signing key\"." instead of the raw enum, so production users see actionable guidance. (#1177) - samples/android-demo/STREETSCAPE_SETUP.md: New "Play App Signing key (production blocker — issue #1177)" section. Documents the runbook to add the Play App Signing SHA-1 (post-resign, NOT the upload key) to the Cloud API key restrictions. Propagation is ~1 min; no new AAB cut needed once Play App Signing key SHA-1 is whitelisted. - .claude/scripts/verify-arcore-key.sh: New CI guard, wired into play-store.yml. Fails fast if ARCORE_API_KEY secret is missing, if build.gradle no longer injects the arcoreApiKey manifest placeholder, or if AndroidManifest.xml drops the ${arcoreApiKey} reference. Catches the silent-regression class that ships an AAB with an unwired Cloud key. Verified-fixed closures (issues stay open just for tracker hygiene — code already lives on main): - #1097 spherePlaneResponse wrong contact point on negative side — fixed in CollisionResponse.kt line 89 (contactPoint = center - planeNormal * signedDist, projects along the original unflipped normal). JVM regression test `spherePlaneResponseContactPointLandsOnPlaneOnEitherSide` pins it. - #1178 AR Terrain & Rooftop Anchors fail under R8 — fixed in PR #1190 already on main. Consumer rules keep com.google.android.gms.location.**, common.api.**, tasks.**. Verified by dexdump on this hotfix's assembleRelease APK: 102 location classes survive R8. - #1061 AR rendering quality umbrella — all P0/P1 sub-issues closed: - #1062 baseline-relative light apply (ARScene.kt line 817, AtomicReference) - #1063 neutral IBL fallback (createAREnvironment loads neutral_ibl.ktx) - #1064 HDR specular filter default true (LightEstimator.kt line 128) - #1067 AR exposure aligned to v4.1.0 3D defaults - ENVIRONMENTAL_HDR is the default LightEstimationMode (ARScene.kt line 482) Remaining sub-issues #1065 (recording resolution) and #1066 (camera-stream double-gamma) stay open as P1 polish for v4.4. Build verification: - ./gradlew :sceneview:compileReleaseKotlin :arsceneview:compileReleaseKotlin → BUILD SUCCESSFUL - ./gradlew :samples:android-demo:testDebugUnitTest :arsceneview:testDebugUnitTest → BUILD SUCCESSFUL - ./gradlew :samples:android-demo:assembleRelease → BUILD SUCCESSFUL (R8 + minify + shrinkResources) - 30+ version locations bumped 4.3.2 → 4.3.3 (sync-versions.sh: 0 errors / 0 warnings). - bash .claude/scripts/impact-check.sh → PASS (1 unrelated cross-platform warning). Co-authored-by: Thomas Gorisse <thomas.gorisse@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
thomas-gorisse
added a commit
that referenced
this pull request
May 15, 2026
…-14 batch (#1120) (#1342) Three PRs from yesterday's 15-PR rendering batch shipped without JVM regression-pin tests. Add focused pins for each: - #1086 — `environmentalHdrSpecularFilter = true` default. Source pin so a future "default-off for perf" PR fails the build instead of silently reverting it. - #1091 — cubemap upload callback. The original double-close fix evolved (CORR-B #1094 + #1102 hoist) into the instance-scoped `uploadCompletedCallback`. Pin the current contract: callback is the hoisted ref, body is a pure `uploadInFlight = false` flip, and it never closes `arImages` again (the #1090 double-close). - #1095 — reflection check that all 7 `LightEstimator` cross-thread toggles carry `@Volatile`. #1088 (ARDefaultCameraNode EV11.6) is already pinned by the existing `ARDefaultCameraNodeTest.kt`, so no new test was needed there. Test-only change — no production code touched. Closes #1120 Co-authored-by: Thomas Gorisse <thomas.gorisse@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1064.
The toggle exists, just defaulted wrong. Filament's
IndirectLight.reflections()expects a roughness-prefiltered mip chain — feeding it the raw ARCore cubemap means every roughness lookup samples mip 0 → mirror-like reflections regardless of the material's roughness slider, oversaturated highlights, color shifts on metallic/glossy models.Flip the default to
true(the toggle's existence already documented this is the right behavior — only the default was wrong).Cost: one-time prefilter pass per cubemap update (~5–15 ms on Pixel 9). ARCore updates the HDR cubemap ~1 Hz → net AR frame time impact under 0.5 ms.
QA
:arsceneview:compileReleaseKotlingreen:arsceneview:testDebugUnitTest11 LightEstimatorTest + 3 ARMainLightBaselineMultiplyTest greenmodel-viewer-arwith the helmet, verify reflections vary visibly with the roughness slider (pre-fix: identical mirror-like at all values)🤖 Generated with Claude Code