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

Remove the per-item mask and complex clip feature #1377

Closed
mrobinson opened this issue Jun 14, 2017 · 5 comments
Closed

Remove the per-item mask and complex clip feature #1377

mrobinson opened this issue Jun 14, 2017 · 5 comments

Comments

@mrobinson
Copy link
Member

@mrobinson mrobinson commented Jun 14, 2017

The idea is that since masking and complex clips are comparatively uncommon we can rely on the ClipScrollTree for them. This will allow us to simplify the clipping code quite a bit and allow deduplication of clips to happen as soon as items are added to the clip scroll tree. The local clip rectangle would remain, since it is handled differently internally and is much more common. This should also reduce the size of the display list.

Issues necessary to fix to make this happen:

@mrobinson
Copy link
Member Author

@mrobinson mrobinson commented Jun 14, 2017

@mrobinson
Copy link
Member Author

@mrobinson mrobinson commented Jun 16, 2017

So one potential issue with this, that I didn't notice before, is with the extra_clips. These are currently used for box shadows:

        let extra_clips = [ClipSource::Complex(bs_rect,
                                               border_radius,
                                               bs_clip_mode),
                           // Clip the outside of the box
                           ClipSource::Complex(*box_bounds,
                                             border_radius,
                                             box_clip_mode)];

I believe this means that every box shadow is going to have a node in the clip scroll tree. This might not be too bad, but we need to be careful not to introduce performance regressions now and in the future by accidentally relying on a slow ClipScrollTree.

@kvark
Copy link
Member

@kvark kvark commented Jun 16, 2017

@mrobinson perhaps, the right way to think about it is within the context of shadow in general. I.e. have a node specifying the shadow to be applied for all children (including text and other things).

@mrobinson
Copy link
Member Author

@mrobinson mrobinson commented Jun 24, 2017

With #1412 landing, this feature is gone from the API, but still exists internally to handle the extra_clips property. I think the next step is to move everything to reference frame coordinates internally in order to reduce the total number of ClipScrollGroups. Then we can move extra_clips to the ClipScrollTree and close this.

@mrobinson
Copy link
Member Author

@mrobinson mrobinson commented Jun 29, 2017

I think we can close this for now, since it is no longer clear that this is the direction we want to go. Instead we will move toward simplifying the kind of clips that the API will accept.

@mrobinson mrobinson closed this Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.