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

Make dashed-border rendering more similar to Gecko / other browsers. #3039

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@emilio
Member

emilio commented Sep 10, 2018

This mostly focuses on non-cornered dashed borders.

The worst case of this algorithm is the one where the borders are rounded and the space between corners (the solid segment) is small but non-zero.

Thus I'm a bit on the fence on the fourth commit... I could preserve the current algorithm for the rounded corner cases.

The tl;dr is that this makes a dashed segment always start with half a dash, which matches other browsers (modulo a TODO where other browsers start with the whole segment, and border-radius when each browser does something completely different so I suspect we may be a bit more lenient).

I think we should do this for now if it doesn't cause problems, though it's trivial to extend it to do the same as Gecko for discontinued borders, etc. It's just a bit more annoying because instead of passing a flag I need to pass the offset around in a new float value.

I ended up shifting the brush in brush_picture.glsl, where the rest of the repeating code is handled. That being said, I've done the transformation in local space, but there may be a better way to do it.

There are some cleanups to the border code that I can do if this is a decent / acceptable approach.


This change is Reviewable

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 10, 2018

Contributor

It's probably not ideal that we need some border-specific code / flags in the common brush.glsl code, which I guess is what you were talking about on IRC?

What we actually do for dashes in corners now

instances.push(BorderInstance {

is generate an instance for each dash / dot. We do this for corners because the size of the dash / dot varies with each instance as traced around the corner.

I think what we probably want to do is something similar for dots / dashes in the border edge segments. This would give us complete control over specifying a local rect for each of the dashes, without affecting the common brush code.

Previously, we wanted to do it in the shader to avoid generating a lot of instances per frame, but we now cache border segments to the texture cache, and re-use them between frames, so this isn't really a concern now.

Having said all that, given our current deadlines and bug list - it's probably fine to merge a solution like this and do the work above as a follow up later on - thoughts?

Contributor

gw3583 commented Sep 10, 2018

It's probably not ideal that we need some border-specific code / flags in the common brush.glsl code, which I guess is what you were talking about on IRC?

What we actually do for dashes in corners now

instances.push(BorderInstance {

is generate an instance for each dash / dot. We do this for corners because the size of the dash / dot varies with each instance as traced around the corner.

I think what we probably want to do is something similar for dots / dashes in the border edge segments. This would give us complete control over specifying a local rect for each of the dashes, without affecting the common brush code.

Previously, we wanted to do it in the shader to avoid generating a lot of instances per frame, but we now cache border segments to the texture cache, and re-use them between frames, so this isn't really a concern now.

Having said all that, given our current deadlines and bug list - it's probably fine to merge a solution like this and do the work above as a follow up later on - thoughts?

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 11, 2018

Member

Try is green and this of course fixes #2828 and such.

Need to cleanup and remove the now unused flags and such which I'll do tomorrow, but @gw3583 does this look reasonable to you?

Member

emilio commented Sep 11, 2018

Try is green and this of course fixes #2828 and such.

Need to cleanup and remove the now unused flags and such which I'll do tomorrow, but @gw3583 does this look reasonable to you?

@emilio

This comment has been minimized.

Show comment
Hide comment
Member

emilio commented Sep 11, 2018

@kvark

kvark approved these changes Sep 11, 2018

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @emilio)


webrender/res/brush_image.glsl, line 99 at r1 (raw file):

        //       works. That assumption may not hold if this
        //       is used for other purposes in the future.
        if ((brush_flags & BRUSH_FLAG_SEGMENT_REPEAT_X) != 0)

nit: I don't think this formatting changes are good. Better be consistent with both Rust and the rest of GLSL code and always use the brackets


webrender/res/cs_border_segment.glsl, line 65 at r1 (raw file):

#define CLIP_NONE        0
#define CLIP_DASH_CORNER 1

nit: can we put the DASH_* types together?


webrender/res/cs_border_segment.glsl, line 367 at r1 (raw file):

            // |xxx|   |   |xxx|
            // +---+---+---+---+
            bool in_dash = is_vertical

nit: float pos = is_vertical ? vPos.y : vPos.x;


webrender/src/border.rs, line 247 at r1 (raw file):

#[derive(Debug, Clone)]
struct BorderCornerClipSource {
    // XXX this makes no sense as a name for dashed borders now that it

what exactly makes no sense now? max_clip_count? BorderCornerClipSource?


webrender/src/border.rs, line 369 at r1 (raw file):

                let mut current_length = 0.;

                dot_dash_data.reserve(max_clip_count / 4);

this should have +1 like the loop does


webrender/src/border.rs, line 992 at r1 (raw file):

        BrushSegment::new(
            image_rect,
            /* may_need_clip_mask = */ true,

asks to be an enum :)

Make dashed-border rendering more similar to Gecko / other browsers.
This commit ensures that dashed segments always start and end with half a dash.

We should probably do better, and apply the condition in the TODO comment. But
this matches existing rendering engines much closer than we do right now.

We also ensure to always draw something in a dashed border corner.
@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 11, 2018

Member

Thanks for the review @kvark!

Addressed comments and squashed the commits. I'll cleanup BorderCornerClipSource as a followup I think. Shouldn't be really hard.

Member

emilio commented Sep 11, 2018

Thanks for the review @kvark!

Addressed comments and squashed the commits. I'll cleanup BorderCornerClipSource as a followup I think. Shouldn't be really hard.

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio
Member

emilio commented Sep 11, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

📌 Commit 7717221 has been approved by kvark

Contributor

bors-servo commented Sep 11, 2018

📌 Commit 7717221 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

⌛️ Testing commit 7717221 with merge 02f14d0...

Contributor

bors-servo commented Sep 11, 2018

⌛️ Testing commit 7717221 with merge 02f14d0...

bors-servo added a commit that referenced this pull request Sep 11, 2018

Auto merge of #3039 - servo:dashed-borders, r=kvark
Make dashed-border rendering more similar to Gecko / other browsers.

This mostly focuses on non-cornered dashed borders.

The worst case of this algorithm is the one where the borders are rounded and the space between corners (the solid segment) is small but non-zero.

Thus I'm a bit on the fence on the fourth commit... I could preserve the current algorithm for the rounded corner cases.

The tl;dr is that this makes a dashed segment always start with half a dash, which matches other browsers (modulo a TODO where other browsers start with the whole segment, and border-radius when each browser does something completely different so I suspect we may be a bit more lenient).

I think we should do this for now if it doesn't cause problems, though it's trivial to extend it to do the same as Gecko for discontinued borders, etc. It's just a bit more annoying because instead of passing a flag I need to pass the offset around in a new float value.

I ended up shifting the brush in `brush_picture.glsl`, where the rest of the repeating code is handled. That being said, I've done the transformation in local space, but there may be a better way to do it.

There are some cleanups to the border code that I can do if this is a decent / acceptable approach.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3039)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 12, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 02f14d0 to master...

Contributor

bors-servo commented Sep 12, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 02f14d0 to master...

@bors-servo bors-servo merged commit 7717221 into master Sep 12, 2018

4 of 6 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
code-review/reviewable 3 files, 6 discussions left (emilio, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
Taskcluster (push) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@kvark kvark deleted the dashed-borders branch Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment