Repository navigation
fix: Codex review findings on #480, #484, #485, #490 (relinker prefixes, Metal depth retirement, null degamma images) - #498
Conversation
… prefixes A scalar compare carrying an address-size override (67 C5 FA C2 ..., 67 F3 0F C2 ...) or with F2/F3 on either side of another legacy prefix was not decoded, so it stayed in its Rosetta block and could still lose the later load of its destination. Legacy prefixes are now read in any order, the last F2/F3 being the mandatory one; F2/F3 before a VEX escape is still not a compare. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…yered slice on Metal A multisampled color target now retires the depth surfaces over all of its fragments (bytes * samples), as MetalColorSurface::OverlapsMappings already measures it. Each extra slice of a layered depth draw retires the surfaces over its depth and stencil planes, not only one at exactly its depth address. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A null T# (TYPE=0 or zero base) reads zero whatever its other words say, so a null image left with an RGBA8_UINT format under a FORCE_DEGAMMA sampler no longer fails the one-slice-volume check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An R/B-swapped UNORM and sRGB texture sampled under FORCE_DEGAMMA keeps its mapping once. Measured: a Metal view of a view takes its swizzle relative to the root texture, so passing the parent's swizzle is right and an identity swizzle drops the mapping. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd6737bd1c
ℹ️ 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".
| slice.slice += layer; | ||
| auto surface = depthCache->Acquire(slice); | ||
| std::erase_if(depthSurfaces, [&](const auto& other) { return other != surface && other->Target().address == slice.address; }); | ||
| RetireDepthSurfaces(depthSurfaces, slice, surface.get()); |
There was a problem hiding this comment.
When a layered stencil-only target is used (DB_Z_INFO.FORMAT == 0), its depth address starts at zero, but the loop above unconditionally offsets it for each extra slice; this new range-based retirement then treats that fabricated low address as a real depth plane and can evict unrelated resident depth surfaces whose ranges overlap it. Keep a zero depth address unchanged and skip retirement for that absent plane, as is already done for a zero stencil address.
Useful? React with 👍 / 👎.
…ts layered slices The extra slices of a layered draw offset the depth address even when it is zero (a stencil-only target), and the range retirement then treated that fabricated address as a depth plane. DepthArraySlice leaves an absent plane at zero. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
Triage of unaddressed Codex connector findings on merged PRs #480, #484, #485 and #490. None of them was fixed upstream: upstream main has no Metal backend and no
materializeForcedDegamma, and #480 is fork-only. Each real finding first got a test that failed on main.67 C5 FA C2 ...,67 F3 0F C2 ...) is not decoded67and F2/F3 prefixes in any order, and the last F2/F3 is the mandatory one. Before the fix, these compares were left un-isolated in their Rosetta block. They were not misdecoded, so the original load-loss hazard could come back. The moved stub keeps the EIP-relative target. Test:amd64_only_converter_tests(decode + matcher relocation).bytes * samples, the same footprintMetalColorSurface::OverlapsMappingsuses. Test:metal_utility_testsDepthResource. It failed on the old range.metal_utility_testsDepthResource.ImagePlaneBiasOnSingleLevelVolumedraws a TYPE=0 RGBA8_UINT image under FORCE_DEGAMMA and reads zero. It threw "requires a one-slice volume" on main.swizzle:view.swizzle, an R/B-swapped UNORM T# under FORCE_DEGAMMA samples correctly swapped. The suggested identity swizzle drops the mapping: the new test fails with R reading B's value. The test now pins that behaviour.DepthArraySlicekeeps an absent plane at zero. Test:metal_utility_testsDepthResource. It failed without the guard.Verification:
amd64_only_converter_tests,decoder_length_tests(relinker-only config);metal_utility_tests, allanyps5_metal_driver_semantics_*,anyps5_metal_render_state,anyps5_metal_msaa_depth(Metal guest-replay config);rosetta_engine+rosetta_cpuid_tests(x86_64 Rosetta config). All pass. One 300 s map smoke reached the 3D map (map generation started by about 86 s) with no fault, and the last frame renders terrain and HUD.🤖 Generated with Claude Code