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

Extensions UBOs #9001

Merged
merged 47 commits into from
Jul 4, 2024
Merged

Extensions UBOs #9001

merged 47 commits into from
Jul 4, 2024

Conversation

felixpalmer
Copy link
Collaborator

@felixpalmer felixpalmer commented Jul 3, 2024

For #8997

Change List

  • Port BrushingExtension
  • Port ClipExtension
  • Port CollisionFilterExtension
  • Port FillStyleExtension
  • Port PathStyleExtension
  • Port TerrainExtension

name: 'clip-fs',
fs: shaderFunction
const shaderModuleFs: ShaderModule<ClipModuleSettings> = {
name: 'clip',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any risk sharing the name between the modules?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any risk sharing the name between the modules?

I am not quite sure what the context is but I would avoid using same name for two modules if that is the question.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Luma assumes that the uniform block will be called ${moduleName}Uniforms - which for starters means we cannot use clip-fs.

It also means that if we cannot share the same uniformBlock across multiple modules. Perhaps we should add an optional ShaderModule.uniformBufferName in luma?

Copy link
Collaborator

Choose a reason for hiding this comment

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

[Luma assumes] the uniform block will be called ${moduleName}Uniforms

Yes... was really a hack, not an intentional design. There are some restrictions to using the same name for both block names in shaders - so I added this logic to unblock things.

It also means that if we cannot share the same uniformBlock across multiple modules. Perhaps we should add an optional ShaderModule.uniformBufferName in luma?

Agreed, at some point we will want to revisit that. Perhaps a more explicit system would be better as you suggest.

which for starters means we cannot use clip-fs.

If the hyphen is the problem, we could easily add a hyphen-case to camelCase converter or just call the module clipfs?

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.

LGTM.

modules/extensions/src/brushing/brushing-extension.ts Outdated Show resolved Hide resolved
name: 'clip-fs',
fs: shaderFunction
const shaderModuleFs: ShaderModule<ClipModuleSettings> = {
name: 'clip',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any risk sharing the name between the modules?

I am not quite sure what the context is but I would avoid using same name for two modules if that is the question.

Base automatically changed from felix/data-filter-ubo to master July 4, 2024 14:49
@felixpalmer felixpalmer changed the title WIP: Extensions UBOs Extensions UBOs Jul 4, 2024
@felixpalmer felixpalmer merged commit f8723cb into master Jul 4, 2024
2 checks passed
@felixpalmer felixpalmer deleted the felix/extensions-ubos branch July 4, 2024 15:05
@coveralls
Copy link

Coverage Status

coverage: 89.253% (+0.02%) from 89.231%
when pulling cc2d5e3 on felix/extensions-ubos
into 2195b8b on master.

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.

3 participants