arkavo-org / arkavo-org/VRMMetalKit

GLTFMetalKit: actor-isolate GLTFRenderer or document single-threaded contract

Open
#252 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.