fix: track native texture copy usage - #312
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
kolkov
left a comment
There was a problem hiding this comment.
Excellent work — this fills a gap our own validation agents identified (copy commands not recording texture usage). The atomic preflight pattern (prepare → validate all → commit) is clean and well-tested.
Code quality: enterprise level. 1,102 LOC tests, 100% patch coverage, 7 atomic-conflict scenarios, full lifecycle test through Submit. Consistent with our ADR-060 patterns.
ADR-060 compatibility: changes to core/track/texture.go and core/track/buffer.go are purely additive (ReplaceUsage). encoder_native.go restructures copy commands — consistent with our tracker direction.
Prerequisites (from your description): confirmed real but pre-existing, not introduced by this PR. The scope recording is independently correct.
Three items to address:
-
CopyBufferToBufferconsistency. The PR changesrecordBufferUsagereturn type toboolbutCopyBufferToBuffer(lines 181-182) still ignores the return value and proceeds to the HAL call on conflict. The other three copy commands use the new atomic pattern. Either updateCopyBufferToBufferto match, or note the exclusion in the PR description. -
Rust reference comments.
RecordTextureUsageandReplaceTextureUsageincore/command.goshould referencewgpu-core command/transfer.rsfor consistency withRecordBufferUsage. -
ReplaceUsagedoc comment. A note that it is intended for use after preflight validation only (unconditional write, no conflict check) would clarify the contract.
None of these are architectural — happy to approve once item 1 is addressed.
c78a110 to
2228fe8
Compare
8864a06 to
92f8974
Compare
kolkov
left a comment
There was a problem hiding this comment.
All three items from the previous review are addressed:
-
CopyBufferToBuffer atomic preflight — now uses
recordCopyBufferUsageswith prepare → validate → commit pattern, consistent with the other 3 copy commands. Failed copies no longer clone ResourceRefs. -
Rust reference comments — added on
RecordTextureUsageandReplaceTextureUsage. -
ReplaceUsage doc comment — added on both
BufferUsageScope.ReplaceUsageandTextureUsageScope.ReplaceUsage, clarifying post-preflight intent.
Test quality is excellent — 5 test files covering atomicity invariants (7 sub-tests for all copy command combinations), resource lifecycle through encoding/submit/retirement, guard branches, and edge cases.
LGTM.
|
Hey @besmpl — all review items addressed, CI green. Please double-check everything on your side and mark "Ready for review" when you're confident. We'll merge with |
|
Merged — thank you for the excellent work on this. The atomic preflight pattern across all 4 copy commands is exactly what we needed, and the test coverage (atomicity invariants, refcount lifecycle, guard branches) is enterprise quality. Apologies for not waiting for you to remove draft — this has been important for our tracker pipeline and we wanted to get it into the next release. All review items were addressed and CI was green, so we went ahead with the squash merge. Also appreciated the extra fixes bundled in: |
* chore: prepare release v0.31.6 - CHANGELOG: add v0.31.6 entry (PR #312 copy usage tracking, docs updates) - AGENTS.md: update version v0.30.22 → v0.31.6 and all dep versions - README.md: goffi v0.6.1 → v0.6.3 - ARCHITECTURE.md: update all dep versions to current - .gitignore: add .backup-investigation/ * fix: exclude nolintlint for browser convert files (CI lint blind spot) CI lint runs on ubuntu (GOOS=linux) — browser files with //go:build js,wasm are invisible. nolintlint flags //nolint:goconst as unused because goconst never runs on these files in CI. Same root cause as Metal lint blind spot (v0.31.3). Widened exclusion to cover all convert_*.go browser files. * fix: complete cross-platform lint exclusions (browser + rust backends) Extend golangci-lint exclusions for all platform-specific files invisible to CI lint runner (ubuntu, GOOS=linux). Known upstream bug (golangci/golangci-lint#3833). Browser (internal/browser/): revive var-naming (ref_ avoids Ref() collision), gocritic, nestif (JS interop nesting). Rust (*_rust.go): nolintlint for invisible //nolint directives. Browser mapped_range: nolintlint for //nolint:gosec. Verified: 0 issues on all 4 targets (Windows, Linux, macOS, WASM).
Summary
Why
The native encoder tracked buffer usage for texture↔buffer copies but never recorded texture usage. Texture↔texture copies recorded neither endpoint. Submit-time tracking therefore could not generate transitions from an already tracked Resource or ColorTarget state to the transfer layouts required by Vulkan/DX12.
Copy commands now validate every endpoint first and commit their scopes only when the complete operation is valid. Failed operations leave scopes, resource refs, submit-validation sets, and the HAL command stream untouched. Source/destination buffers are retained through queue retirement.
Verification
go test ./...go test -race . ./core ./core/trackgo build ./...go vet . ./core ./core/trackgofmtandgit diff --checkKnown prerequisite dependencies
This PR intentionally fixes command-scope recording, not the complete queue transition engine. It remains draft because correct emitted transitions require coordinated state-model and submission-order work:
Uninitialized. Registration cannot stand alone: tracker state must be removed exactly once before tracker-index reuse, swapchain acquire/present must account for each physical image, and pendingWriteTexture/explicit-transition paths must keep the tracker synchronized.Until those dependencies are solved, this PR's claims are limited to correct atomic copy-scope and resource-lifetime recording.
Review follow-up (2026-08-12)
CopyBufferToBuffernow validates both endpoint usages atomically before mutating scope state, retaining refs, or recording HAL commands.ReplaceUsageand added the matching wgpu-corecommand/transfer.rsreferences.The PR intentionally remains draft for the broader queue-transition prerequisites already listed above.