arkavo-org / arkavo-org/VRMMetalKit
GLTFMetalKit: actor-isolate GLTFRenderer or document single-threaded contract
- Dominant language
- Swift
- Stars
- 6
- Forks
- 2
- Avg merge
- 18h 51m
- Merged PRs (30d)
- 26
Description
## Context
Reviewer feedback on PR #241 (Minor #12):
> `@unchecked Sendable` on `GLTFRenderer`. The renderer holds mutable `var environment: GLTFEnvironment` — a public read-write property. Concurrent reads-during-writes on different threads is the kind of bug that won't be caught by the compiler with `@unchecked`. Either make `environment` immutable post-init and add a `setEnvironment` method that swaps under a lock, or document explicitly that the renderer is single-threaded.
PR #241's review-feedback commit added DocC noting the single-threaded contract. This issue tracks the stronger option: actor-isolate the renderer or guard `environment` swaps under a lock so the type's `Sendable` claim is structurally honest.
## What's needed
Option A: Convert `GLTFRenderer` to an `actor`.
- Pro: Compiler-enforced isolation, removes `@unchecked Sendable` claim.
- Con: Every public method becomes `async`, ripples through the call sites + tests.
Option B: Make `environment` private. Expose a `setEnvironment(_ env: GLTFEnvironment) { lock.withLock { ... } }` method that swaps under a lock. Reads inside encode methods take the same lock.
- Pro: Smaller API change. Concurrency-safe.
- Con: Lock contention per draw call (lock per encode).
Option C: Use Swift's `@unchecked Sendable` with a documented contract (current state — done by the PR's review-feedback commit). Lowest cost; relies on caller discipline.
Recommendation: Option B is the right shape for a render loop — single writer (env-update path) + multiple readers (encoder path) is a classic reader-writer scenario, but with one writer-per-frame and many reads-per-frame the simpler `NSLock` is fine.
## Acceptance
- `GLTFRenderer` no longer needs `@unchecked Sendable` (or keeps it but with structural guarantees behind the unchecked claim).
- 1466 tests pass.
- A new test exercises a concurrent encoder + environment swap, catching the race that the current API allows.
## Estimate
Option B: ~1.5 hours including the concurrency test. Option A: ~half a day given the API surface ripple.
Source: surfaced in PR #241 review.
Contributor guide
Research direction
Start by reading GLTFRenderer, its environment property, encode methods, and the call sites and tests affected by the actor or locking options. Review the concurrency behavior described in PR #241, then run the existing 1466-test suite. Done means the Sendable claim is structurally safe and a concurrent encoder/environment-swap test passes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- computer-graphics, testing
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100