Skip to content

feat: retain custom materials assigned to asset primitives - #356

Closed
wildseansy wants to merge 2 commits into
margelo:mainfrom
PocketConservatory:codex/asset-owned-materials
Closed

wildseansy wants to merge 2 commits into
margelo:mainfrom
PocketConservatory:codex/asset-owned-materials

Conversation

@wildseansy

@wildseansy wildseansy commented Oct 13, 2026 •

Copy link
Copy Markdown

Implements the proposal in #352.

Add FilamentAsset.setMaterialForPrimitive(entity, primitiveIndex, material) to assign a custom material's default instance to a primitive of a loaded glTF asset. Validate asset membership and primitive bounds before assignment.

The asset retains the native material owner independently of its JS wrapper. Asset disposal destroys the glTF renderables first, then releases retained materials outside the engine mutex. Releasing a Material wrapper while the model is visible therefore does not destroy a still-attached material.

The API runs on the renderer worklet and requires a material from the same engine. Assigned default instances are shared; replacements remain retained until asset teardown. These semantics and native lifetime-check steps are documented in the asset-loading guide.

This is separate from #343, which fixes duplicated instances/textures created by RenderableManager.changeMaterialTextureMap(). This PR does not modify or claim to fix that path.

Validation

  • Complete contributor command yarn check-all passes with Yarn Classic 1.22.22 and clang-format 16, using dependencies installed from the frozen upstream Bun lockfile. Separate full yarn lint and yarn typescript also pass; no source formatting changes were required.
  • The same native ownership/assignment implementation has been used in a downstream Android arm64 build and limited iOS simulator rendering, with bounded scene replacement/teardown testing.
  • Both changed C++ translation units pass Android NDK arm64 syntax compilation against the upstream checkout and consumer dependency headers.
  • Changed C++ passes clang-format 16; changed TypeScript passes focused ESLint/Prettier; git diff --check passes.
  • The guide's immediate-wrapper-release, replacement, and two-assets-sharing-one-material scenarios are proposed manual regression checks, not newly executed tests. Physical-device validation and the additional material-lifetime scenarios remain unverified.

API and ownership feedback is welcome on #352. There are no downstream engine paths, release tooling or assets in this change.

The documentation example now guards assets without renderable entities; the example was also checked with the upstream TypeScript configuration.

Fresh upstream Android example validation

A temporary QA checkout based on upstream 0bcf507 combined the three independent PR changes (#354, #355, #356). The unmodified AppExampleFabric app (React Native 0.83.1) built for Android arm64 in 3m26s. Its native engine/Bullet binaries came from the verified upstream 1.11.0 package; tracked engine headers matched byte for byte. This is a combined integration build, not three separate example APK builds.

On an Android 16 arm64 emulator, Home → CameraPan → Home → AnimationTransitions → Home rendered both native models. The process remained alive and its crash buffer was empty during this bounded run. Initial blank startup resolved after restarting with Metro ready, without source changes.

The example's built-in HybridObject smoke test still reports an invalid-enum error when reading the initial enum in unchanged upstream test code (RNFTestHybridObject leaves that field uninitialized). StrictMode warnings also remain. This result establishes bounded rendering/navigation, not a clean pass of every runtime test, the newly documented material-lifetime cases, or physical-device/iOS validation.

Upstream CI

GitHub currently reports action_required for this fork's workflows. A repository maintainer must approve those runs; local checks above have completed. This PR remains draft pending upstream CI.

@wildseansy

Copy link
Copy Markdown
Author

🍋 Code Reviewer / QA: LGTM at d1c374b; no blockers found. Ownership survives wrapper release and is released after renderable teardown; threading, same-engine and replacement-retention constraints are documented. Syntax/static checks and prior consumer testing are distinguished from the proposed, unexecuted lifetime scenarios; CI needs maintainer approval.

@wildseansy

Copy link
Copy Markdown
Author

🍋 Code Reviewer / QA: LGTM through 3978f17; the documentation now safely handles assets without renderable entities. A temporary branch combining #354–356 built upstream AppExampleFabric (RN 0.83.1, Android arm64 debug); CameraPan and AnimationTransitions rendered and returned to Home on an Android 16 emulator without an observed app crash. This bounded smoke check did not exercise the new material-lifetime scenarios. The example's existing HybridObject self-test error and StrictMode warnings remain separate limitations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant