-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(core): Enable shader/pipeline caching for attached WebGL devices #9971
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
base: master
Are you sure you want to change the base?
Changes from all commits
de7b5f2
d5382c4
45c2ab7
720f737
4665384
6e45bf6
eb86bd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -373,6 +373,10 @@ export default class Deck<ViewsT extends ViewOrViews = null> { | |
| const userOnResize = this.props.deviceProps?.onResize; | ||
|
|
||
| deviceOrPromise = webgl2Adapter.attach(props.gl, { | ||
| // Enable shader and pipeline caching for attached devices (matches _createDevice defaults) | ||
| // Without this, interleaved mode (e.g., MapboxOverlay) creates new pipelines every frame | ||
| _cacheShaders: true, | ||
| _cachePipelines: true, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's put this below the spread in case a user wants to customize their device props. By default
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, thank you so much @chrisgervang! That's a great catch: I've gone ahead and made the required change. I've also gone ahead and manually verified the changes via the Just as a slight note for anyone else wanting to verify the change: I believe that the performance impact scales with scene complexity and interaction frequency. As the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh my apologies, I misread your code. Your original code allowed the user to customize the props and now it forces the caching. Let's revert to your original version, please.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to hear the example is working!
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hahaha, no worries! Changes reverted and pushed. |
||
| ...this.props.deviceProps, | ||
| onResize: (canvasContext, info) => { | ||
| // Manually sync drawing buffer dimensions (canvas is externally managed) | ||
|
|
||
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.
Duplicated caching defaults across device creation paths
Low Severity
The
_cacheShaders: trueand_cachePipelines: truedefault values are now duplicated in two separate locations: thewebgl2Adapter.attach()call and the_createDevice()method. While the comment notes it "matches _createDevice defaults," this relies on manual synchronization. Extracting these defaults to a shared constant would ensure consistent behavior if defaults need to change.Additional Locations (1)
modules/core/src/lib/deck.ts#L865-L867