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

shadow module UBO #9054

Merged
merged 11 commits into from
Aug 1, 2024
Merged

shadow module UBO #9054

merged 11 commits into from
Aug 1, 2024

Conversation

felixpalmer
Copy link
Collaborator

For #8997

Change List

  • Add UBOs for shadow module
  • Remove residual UniformTypes instances

@felixpalmer felixpalmer requested review from donmccurdy and ibgreen and removed request for donmccurdy July 30, 2024 10:32
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.

If we can't sort the layer.ts dependency creep here, let's add it to our adit log for later discussion. https://docs.google.com/document/d/1rBkxgYd5vTKjDC9AHEd3jbrhZgtbQH6TYxaBOgqIDGI/edit

@@ -1079,6 +1079,15 @@ export default abstract class Layer<PropsT extends {} = {}> extends Component<
const {modelMatrix} = this.props;
this.setModuleParameters(moduleParameters);
const {
// shadow
Copy link
Collaborator

Choose a reason for hiding this comment

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

Concern:

  • That is a lot of individual imports, kind of pollutes to have such a big surface area from a leaf functionality into a core file.
  • Same for terrain I suppose though I didn't catch it then.
  • Is there a way things can be made more decoupled or pluggable. Some function to get the props, instead of requiring layer class to be aware of them all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree, I have a TODO to review it once the UBO migration is complete. I chose to do it here as this is where setModuleParameters is being invoked in the same way.

Some of the other modules are already self-contained, like the MaskExtension, so I expect we will be able to tidy this up. First though I would like to remove all the updateModuleSettings calls before potentially changing the data flow in the code.

I have added this point to the Audit Log as suggested

@@ -1,5 +1,5 @@
import type {ShaderModule} from '@luma.gl/shadertools';
import {project, UniformTypes} from '@deck.gl/core';
import {project} from '@deck.gl/core';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a personal preference nit: This <module>/shader-module.ts naming scheme leaves us with a lot of files with the same name. I find that confusing when searching. Would recommend calling this mask-shader-module.ts or something more unique.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No strong opinions here, but for me the fuzzy searching works for the whole path

Screenshot 2024-07-31 at 15 11 24

Copy link
Collaborator

Choose a reason for hiding this comment

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

First: I am not worried about this, leaving it is fine.

That said (bikeshed mode on):

  • I looked and was happy to see that the other files in the directory have the name of the extension (mask-) in their name. So why not this file too?
  • Seems like two different naming philosophies are in play at the same time?
  • At least the naming scheme seems consistent the various new extensions...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While we're bikeshedding, for whatever reason the shader-module files have an anonymous default export whereas the others do not:

class MaskPass extends LayersPass;
class MaskExtension extends LayerExtension;
class MaskEffect implements Effect;

There is a pattern here, where the full superclass is not used for brevity (e.g. we don't have MaskLayersPass). So I would argue if we want to rename the files it would be best to go with MaskModule and then src/mask/mask-module.ts

Copy link
Collaborator

Choose a reason for hiding this comment

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

have an anonymous default export whereas the others do not:

Yes, I have virtually removed all default exports from the internals of the other visgl libraries, as default export are less safe, less compatible than named exports. It would be great to see deck go in the same direction.

So I would argue if we want to rename the files it would be best to go with MaskModule and then src/mask/mask-module.ts

So we do need a name.

  • In luma.gl the shader modules are named with lower case and no suffix: picking, project32 etc.
  • That naming convention is rather presumptive ("we think shader modules are so important that their names don't need any qualifications").
  • Calling it mask would follow luma conventions, but would be a bit "stark" here?
  • Calling it maskModule would be somewhere in-between, perhaps the most reasonable compromise?
  • Calling it MaskModule additionally has the problem that I usually use PascalCase symbols for types, not objects.

@coveralls
Copy link

coveralls commented Jul 30, 2024

Coverage Status

coverage: 89.314% (+0.05%) from 89.261%
when pulling c51204d on felix/shadow-ubo
into 484419d on master.

@felixpalmer felixpalmer merged commit 84f9d2a into master Aug 1, 2024
4 checks passed
@felixpalmer felixpalmer deleted the felix/shadow-ubo branch August 1, 2024 13:47
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