-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[BREAKING] Update gsplat component material API #7749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the GSplat material API to expose and configure material instances directly, replacing the old materialOptions pattern and unifying behavior across resource, instance, and component layers.
- Replace
createMaterialmethods with aconfigureMaterial(material)API on resource classes - Update
GSplatInstanceto accept a providedShaderMaterial(or build a default one), add a getter/setter formaterial, and centralize material configuration - Refactor
GSplatComponentto use amaterialproperty (stored in_materialStore) instead ofmaterialOptions, and apply overrides at instance creation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scene/gsplat/gsplat-sogs-resource.js | Renamed createMaterial to configureMaterial |
| src/scene/gsplat/gsplat-resource.js | Renamed createMaterial to configureMaterial |
| src/scene/gsplat/gsplat-compressed-resource.js | Renamed createMaterial to configureMaterial |
| src/scene/gsplat/gsplat-resource-base.js | Added empty base stubs for configureMaterial and evalTextureSize |
| src/scene/gsplat/gsplat-instance.js | Overhauled to use provided ShaderMaterial, added material getter/setter and configureMaterial |
| src/framework/components/gsplat/component.js | Switched from materialOptions to storing/applying ShaderMaterial directly |
| src/platform/graphics/webgpu/... | Guarded WebGPU calls on window.navigator and added optional chaining |
Comments suppressed due to low confidence (1)
src/scene/gsplat/gsplat-instance.js:127
- [nitpick] There’s no existing unit test covering the new
materialsetter or the path where an externalShaderMaterialis provided. Adding tests would ensure correct assignment, mesh update, and avoid regressions.
set material(value) {
|
The PR should probably be maked as [BREAKING]? |
|
Also please update the examples to work, I suspect some do not. |
Further to #7734, this PR updates the
gsplatcomponent material API.We drop
materialOptionsproperty (which indirectly reconfigured the material) in favour of exposing the material instance directly. This matches therenderandmodelcomponents.The
gsplatcomponent will create a default material if none is provided at construction, but this can then be overridden at will.Notes
GSplatInstance.configureMaterialaccepts a ShaderMaterial instance and configures it with the gsplat data. This function could be useful as public API, but we must still decide how to expose it.gsplatcomponent doesn't clone the material. if it did, then the sort texture would be shared.