-
Notifications
You must be signed in to change notification settings - Fork 563
Support in <Group /> for saveLayerFlags and backdrop #3610
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| export interface CTMProps extends TransformProps { | ||
| export interface SaveLayerProps { | ||
| saveLayerFlags?: SaveLayerFlag; |
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.
SaveLayerFlag being a union is a little awkward since this can actually be a bitset OR of any of the flags. But this problem seems to exist elsewhere in the library. Another option is one boolean property per flag, but then we just have to unpack that everywhere, so it may not be worthwhile.
bb314ab to
0b07047
Compare
| if (isValidElement(layer) && typeof layer === "object") { | ||
| return ( | ||
| <skLayer> | ||
| // keep the saveLayerFlags on whichever node triggers saveLayer |
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.
this is a little awkward, but I tracked everywhere in the graph traversal that ends up calling saveLayer. The Paint props on group must also be provided to saveLayer, so I decided it is safe if I always store the flag prop alongside the layer or other paint props.
BTW I think I found that this <skLayer><skGroup> structure will end up calling saveLayer (for the layer node) and then later calling save via saveCTM (for the group node). Small possibility for optimisation if they can be combined. From what I see the if here is only needed so the paint can be provided as JSX, but I think if the paint element was given as a child to skGroup it might just work out of the box anyway?
Edit: this split creates a surprising gap in test coverage, the execution path when passing an SkPaint or a <Paint/> is quite different. My declarative tests miss one side
|
Hello Mackenzie 🙋🏼♂️ This looks interesting. Can you tell me more about the use-case? Maybe we can start with adding tests for it? We can build the reference results using the imperative API. That would be a good starting point, what do you think? |
|
Hi again @wcandillon ! My use case is for graphical effects that require the use of saved layers. An example is Bloom, which causes bright areas of an image to smear into their surroundings. This is often implemented in postprocessing by copying the result of a pipeline, applying some threshold function, blurring, and then compositing back with the original, e.g. using "plus" or "screen" modes. This is not so easy to implement without a way to save the current framebuffer and re-draw it as a texture. We could draw the scene multiple times, but if we need to run multiple of these postprocessing effects in sequence, there's a quadratic number of draws required. Performance drops quickly, especially if each of the steps involve kernel operations like gaussian blur, which then go onto sample other kernels, et cetera. So saved layers/framebuffers are normally used. Today I’m manually snapshotting the surface and re-drawing it in later steps, but I’d like to move this towards saveLayer and eventually the declarative API. There’s some overlap with
My plan was to support SaveLayerInitWithPrevious in My proposal, then is to add support for Would you be accepting of this feature or see barriers to the implementation? (Support both saveLayerFlags and backdrop as props on |
|
Let's start with writing a few tests for it with the imperative API. Is the imperative API supporting enough options at the moment? I look like it right? |
|
Yes, the imperative API already has everything 👍 This is what I want to implement via declarative: let myPaint: SkPaint;
let myBackdropFilter: SkImageFilter;
let myFlags: SaveLayerFlag;
canvas.saveLayer(myPaint, null, myBackdropFilter, myFlags);
canvas.draw_some_other_elements();
canvas.restore();
So if we add I can come up with some tests, would e2e/Group.spec.tsx with |
|
can you give me 2/3 imperative API example that we can add as tests. This is an example for instance: https://github.com/Shopify/react-native-skia/blob/main/packages/skia/src/renderer/__tests__/e2e/RuntimeShader.spec.tsx#L116. but using the drawOffscreen test API like there: https://github.com/Shopify/react-native-skia/blob/main/packages/skia/src/renderer/__tests__/e2e/Paragraphs.spec.tsx#L135 We will use these are the reference results for the declarative API |
|
@wcandillon I added a few imperative tests with 2 key use cases. (saving a layer through InitWithPrevious OR a backdrop filter, drawing more content on the layer, and then compositing to the previous layer). |
…rop filtering is not
Only supports SkImageFilter objects, not child nodes for now
| export const Group = ({ | ||
| layer, | ||
| backdropFilter, | ||
| saveLayerFlags, | ||
| ...props | ||
| }: SkiaProps<PublicGroupProps>) => { |
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.
it would be really nice if we could pass a child node to the backdropFilter prop. (Similar to the layer prop - that accepts an SkPaint object or a <Paint> node).
I couldn't quite work out how to do this without a big change to the node visitor. I think this dual support for the layer prop is handled by passing the paint node or related nodes as children. They are then implicitly grouped by type in sortNodeChildren.
Because the nodes that would be supported in backdropFilter prop are just image filter nodes, we'd need a way to differentiate between the paint (children) props and this new backdrop prop.
The features still works by passing an SkImageFilter only, we could revisit this later.
… savelayer props for readability
|
@wcandillon I've implemented declarative tests and a passing implementation. Please take a look whenever convenient.
|



No description provided.