You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: retain custom materials assigned to asset primitives - #356
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.
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.
🍋 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.
🍋 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
yarn check-allpasses with Yarn Classic 1.22.22 and clang-format 16, using dependencies installed from the frozen upstream Bun lockfile. Separate fullyarn lintandyarn typescriptalso pass; no source formatting changes were required.git diff --checkpasses.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
0bcf507combined 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 (
RNFTestHybridObjectleaves 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_requiredfor this fork's workflows. A repository maintainer must approve those runs; local checks above have completed. This PR remains draft pending upstream CI.