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

WIP: Use UBOs in project module #8782

Merged
merged 44 commits into from May 7, 2024
Merged

WIP: Use UBOs in project module #8782

merged 44 commits into from May 7, 2024

Conversation

felixpalmer
Copy link
Collaborator

@felixpalmer felixpalmer commented Apr 11, 2024

Background

Status:

  • Requires luma 9.0.12
  • Most shaders & layers working
  • project64 module also ported
  • All tests passing

Change List

  • Update project module to use projectUniforms UBO
  • Update references in all shaders, e.g. project_uDevicePixelRatio -> project.devicePixelRatio
  • Remove project from DEFAULT_MODULES as it needs to be passed in the bindings
  • project64 now passes fp64 matrix as a mat4 uniforms, rather than vec2[16]
  • Test updates

@felixpalmer felixpalmer marked this pull request as draft April 11, 2024 11:19
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Very nice, this is a big part of the lift required to make deck.gl WebGPU ready.

Hacky implementation as luma doesn't handle module dependencies

I am not sure exactly what that means, but is it something we can address in luma.gl to keep this complicated part of deck.gl a little cleaner?

modules/core/src/shaderlib/project/project.glsl.ts Outdated Show resolved Hide resolved
modules/core/src/shaderlib/project/project.glsl.ts Outdated Show resolved Hide resolved
@felixpalmer
Copy link
Collaborator Author

@Pessimistress @ibgreen landing this will be a breaking change for any custom shaders that reference project_uXXXX uniforms. Are we OK with this or do we want to consider automatically rewriting shaders by detecting "project_u" strings and replacing? We could warn that their shaders are using the old syntax and then force the upgrade in 9.1

@ibgreen
Copy link
Collaborator

ibgreen commented Apr 12, 2024

Are we OK with this or do we want to consider automatically rewriting shaders by detecting "project_u" strings and replacing? We could warn that their shaders are using the old syntax and then force the upgrade in 9.1

I hadn't even thought about the possibility of making the uniform buffer change backwards compatible, but it would of course be nice. I recommend looking into the existing functionality in ShaderModules, perhaps it works as is, or can be made to work with small changes:

image

@felixpalmer felixpalmer marked this pull request as ready for review April 19, 2024 12:25
@coveralls
Copy link

coveralls commented Apr 30, 2024

Coverage Status

coverage: 89.82% (+0.01%) from 89.807%
when pulling dac757d on felix/project-ubo
into 1a9dd6f on master.

@felixpalmer felixpalmer merged commit ff9246e into master May 7, 2024
4 checks passed
@felixpalmer felixpalmer deleted the felix/project-ubo branch May 7, 2024 10:15
@ibgreen
Copy link
Collaborator

ibgreen commented May 7, 2024

Epic!

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

4 participants