fix(anari,gltf): resolve rendering and deformation review feedback - #2990
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f28ecf33b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #ifdef USE_SCENE_ENVIRONMENT | ||
| let brdf = brdfSample.rgb; | ||
| let diffuseLight = diffuseSample.rgb; | ||
| let specularLight = specularSample.rgb; |
There was a problem hiding this comment.
Preserve decoding for encoded scene environments
When a SceneRenderer uses sRGB-encoded data stored in linear UNORM textures with MANUAL_SRGB enabled—such as adapting prefiltered image assets loaded by the documented loadPBREnvironment() path—USE_SCENE_ENVIRONMENT now bypasses SRGBtoLINEAR unconditionally. That flag identifies the renderer API, not the textures' encoding, so these environments are interpreted as linear and render substantially too bright; distinguish generated-linear environments from encoded loaded environments instead of skipping conversion for every scene environment.
Useful? React with 👍 / 👎.
| const presentationModel = this.lightingRenderer.textureModel; | ||
| presentationModel.setProps({backgroundTexture: lightingTexture}); |
There was a problem hiding this comment.
Bypass canvas aspect fitting for offscreen resolves
When the supplied framebuffer has a different aspect ratio from the default canvas, setProps() calls BackgroundTextureModel.updateScale(), which computes its scale from the canvas dimensions rather than the framebuffer. The lighting texture already has the framebuffer's dimensions, so the correct scale is 1:1; reusing this screen-presentation model causes the offscreen resolve to crop or stretch one axis. Use a fullscreen copy that is sized from the destination framebuffer rather than the canvas.
Useful? React with 👍 / 👎.
| id: `scene-${options.id}`, | ||
| framebuffer: options.framebuffer, | ||
| clearColor: [background[0], background[1], background[2], background[3] ?? 1], | ||
| clearColor: getPresentedSceneBackground(this.device, options, background), |
There was a problem hiding this comment.
Apply background presentation in the deferred path
When these options are rendered through DeferredSceneRenderer, this color-managed clear is used only if the scene falls back to forward rendering. The compatible deferred path still clears its G-buffer with the raw background, and its lighting shader returns that raw value for untouched pixels, so adding an unsupported material or light changes the background's exposure, tone mapping, and output encoding. Apply the same presentation transform to deferred clear pixels to keep the two paths consistent.
Useful? React with 👍 / 👎.
Goals
Resolve every actionable, currently reproducible review finding on the recently merged ANARI, glTF, animation, PBR, and shared-rendering pull requests without introducing parallel implementations or regressing existing WebGL/WebGPU behavior.
Changes
Animation and retained geometry
Shared physical and deferred rendering
Asset import and documentation
Review coverage
Addresses actionable findings from #2973, #2978, #2979, #2980, #2981, #2982, #2984, and #2985. Non-actionable reviewer mentions, already-fixed threads, and unrelated feedback on an unmerged stacked feature branch are excluded.
Verification
Architecture
Retains loaders.gl as the only file decoder, the existing engine morph and animation runtime, the canonical shared skin/PBR shaders, experimental-owned rendering passes, and the thin ANARI/glTF adapter. No duplicate renderer, material model, animation mixer, decoder, or deformation pipeline is introduced.