-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[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
createMaterial
methods with aconfigureMaterial(material)
API on resource classes - Update
GSplatInstance
to accept a providedShaderMaterial
(or build a default one), add a getter/setter formaterial
, and centralize material configuration - Refactor
GSplatComponent
to use amaterial
property (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
material
setter or the path where an externalShaderMaterial
is 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
gsplat
component material API.We drop
materialOptions
property (which indirectly reconfigured the material) in favour of exposing the material instance directly. This matches therender
andmodel
components.The
gsplat
component will create a default material if none is provided at construction, but this can then be overridden at will.Notes
GSplatInstance.configureMaterial
accepts 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.gsplat
component doesn't clone the material. if it did, then the sort texture would be shared.