deck.gl icon indicating copy to clipboard operation
deck.gl copied to clipboard

WIP: Use UBOs in project module

Open felixpalmer opened this issue 1 year ago • 3 comments

Background

Status:

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

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 avatar Apr 11 '24 11:04 felixpalmer

@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

felixpalmer avatar Apr 12 '24 09:04 felixpalmer

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

ibgreen avatar Apr 12 '24 12:04 ibgreen

Coverage Status

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

coveralls avatar Apr 30 '24 17:04 coveralls

Epic!

ibgreen avatar May 07 '24 14:05 ibgreen

Hi, just checking whether @felixpalmer's question in https://github.com/visgl/deck.gl/pull/8782#issuecomment-2051394539 was resolved — are we comfortable with this PR going into a deckgl v9.0.x patch release, or should we hold it for v9.1? I'm working on publishing v9.0.15 now. Thanks!

EDIT: Discussed offline. Will leave this PR out of v9.0.x and hold it for v9.1.

donmccurdy avatar May 23 '24 20:05 donmccurdy