-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[RFC] ColorFilterExtension #8439
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Chris Gervang <[email protected]>
Signed-off-by: Chris Gervang <[email protected]>
|
||
```js | ||
{ | ||
extensions: [new ColorFilterExtension(hueSaturation), new ColorFilterExtension(brightnessContrast)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer just one extension and instead a list of effects
new ColorFilterExtension([hueSaturation, brightnessContrast])
brightnessContrast: { | ||
brightness: 0.5, | ||
contrast: 0.5 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By importing the module you can easily obtain the props required using Typescript:
brightnessContrast: {
brightness: 0.5,
contrast: 0.5
} as typeof brightnessContrast.props
- Consistency: Ensures standardized performance and appearance. | ||
|
||
**Cons:** | ||
- Limited Flexibility: Confines users to predefined filters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this is too limiting. I don't find importing a shader module from luma and passing it to the ColorFilterExtension
constructor too onerous. It is also what we already have in the PostProcessEffect
} from '@luma.gl/shadertools' | ||
|
||
new Tile3DLayer({ | ||
extensions: [new ColorFilterExtension(brightnessContrast)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PostProcessEffect
has the props passed directly to the constructor. What do you think of the pattern:
new ColorFilterExtension(brightnessContrast, {
brightness: 0.5,
contrast: 0.5
})
Then with types with would get autocompletion on the parameters:
export default class ColorFilterExtension<T extends ShaderModule> extends LayerExtension {
constructor(shaderModule: T, shaderModuleProps: T['props'])
}
|
||
## Conclusion | ||
|
||
After careful consideration of both designs, I'd prefer Design 2: Built-in Shaders in Extension. This design is easier to adopt for a broader range of users, especially those without extensive shader programming experience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My vote is for a variation on Design 1, allowing passing custom shader modules. I think being able to modify the shaders is really powerful and fun and not so difficult for basic color filters
Interesting! This is very similar to what I implemented in deck.gl-raster, where I had a set of shaders that would be compiled and passed in. I think that's similar to your first approach. |
0a083b6
to
eb85da3
Compare
For #
Background
This RFC proposes a new layer extension for
deck.gl
that allows users to easily apply color filters and effects, such as tint, brightness, and saturation, to any layer. The extension utilizes thegetModels
function from each layer and injects aDECKGL_FILTER_COLOR
shader extension into each model.Try it out here.
Change List