Skip to content
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

fix: Support sharing cached Pipelines with different parameters/topology in WebGL #2034

Merged
merged 3 commits into from
Mar 14, 2024

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Mar 13, 2024

For discussion.

Background

  • Make sure we don't create more copies of a Program in WebGL than we need. Before this PR, we create one pipeline for every parameter configuration (just as we must in WebGPU).

Change List

  • Renderpipeline.draw() now accepts optional parameters and topology.
  • These are ignored on WebGPU but on WebGL these are used instead of RenderPipeline.parameters/topology if provided.
  • PipelineFactor no longer includes topology and parameters in hash on WebGL

Notes

  • 4 parameters can be changed in WebGPU should they be handled by this mechanism in parallel with existing mechanism?

Commentary

There is a balance between completely pushing this type of logic down into core and keeping core as a thin GPU API mapping layer.

I have given this quite a bit of thought. By handling some small amounts of device differences like this in the Model, we can keep the complexity of the @luma.gl/core module low and we still hide WebGL/WebGPU differences from applications.

There are a few other differences such as constant attributes and TransformFeedback that that are also handled this way. I feel that this is a good balance, better than the alternatives.

@ibgreen ibgreen changed the title fix: Support sharing Pipelines with different parameters in WebGL (POC) fix: Support sharing cached Pipelines with different parameters/topology in WebGL Mar 13, 2024
// case 'webgl':
// WebGL is more dynamic
// return `${vsHash}/${fsHash}V${varyingHash}BL${bufferLayoutHash}`;
case 'webgl':
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not part of this PR, but should we move the hash logic to each device?

device.getRenderPipelineHash(props: RenderpipelineProps): string

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... Perhaps we could just expose the capabilities instead, perhaps using the device feature system: dynamic-topology-webgl, dynamic-parameters-webgl so that the PipelineFactory could take those into account without switching on device type, and the factory could still own and encapsulate the hashing system.

@ibgreen ibgreen marked this pull request as ready for review March 14, 2024 01:28
@ibgreen ibgreen merged commit ce87eac into master Mar 14, 2024
1 check passed
@ibgreen ibgreen deleted the ib/parameters branch March 14, 2024 01:30
felixpalmer pushed a commit that referenced this pull request Mar 21, 2024
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.

None yet

3 participants