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

Basic implementation of line decoration display items. #1504

Merged
merged 3 commits into from Jul 21, 2017

Conversation

@glennw
Copy link
Member

glennw commented Jul 20, 2017

Includes the commits in #1491 from @gankro.

This provides a basic implementation of the line decoration styles in CSS.

I haven't specifically tried to make the styles (e.g. wavy, dotted) match other browsers, yet. I think it's reasonable to get this reviewed and merged, and then tweak the visuals as a follow up.


This change is Reviewable

Gankra added 2 commits Jul 14, 2017
Contains a dummy backend implementation for basic tests
@glennw
Copy link
Member Author

glennw commented Jul 20, 2017

Sample of the current line styles, including text-shadow blur support:

decorations-suite

@glennw
Copy link
Member Author

glennw commented Jul 20, 2017

@glennw glennw force-pushed the glennw:lines branch from 44c3694 to 1fd1eaa Jul 20, 2017
@kvark
kvark approved these changes Jul 20, 2017
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

float det(vec2 a, vec2 b) {

This comment has been minimized.

@kvark

kvark Jul 20, 2017

Member

is this a cross product? Not clear why det name is chosen.

This comment has been minimized.

@glennw

glennw Jul 20, 2017

Author Member

It's a 2d cross product, I guess. It's just using the same name as the paper the algorithm was taken from.

impl ToGpuBlocks for LinePrimitive {
fn write_gpu_blocks(&self, mut request: GpuDataRequest) {
request.push(self.color);
request.push([pack_as_float(self.style as u32),

This comment has been minimized.

@kvark

kvark Jul 20, 2017

Member

what's the reason to call pack_as_float here as opposed to just casting to f32?

This comment has been minimized.

@glennw

glennw Jul 20, 2017

Author Member

pack_as_float adds 0.5 to ensure that casting back to an int definitely gets the correct value. It's more a defensive measure than anything - perhaps it's not necessary?

@@ -1878,6 +1904,22 @@ impl Renderer {
&target.text_run_textures,
&projection);
}
if !target.line_cache_prims.is_empty() {
// TODO(gw): Technically, we don't need blend for solid
// lines. We could check that here?

This comment has been minimized.

@kvark

kvark Jul 20, 2017

Member

if that becomes a performance concern, we'd need to split those into opaque/non-opaque

This comment has been minimized.

@glennw

glennw Jul 20, 2017

Author Member

Yup

@Gankra
Copy link
Contributor

Gankra commented Jul 20, 2017

Seems good!

I kinda lost track of what you really wanted with the line-dimension specifying stuff. From servo's perspective, boxes + orientation would be easier to send (because of how their logical->real conversion works). Gecko would probably be fine either way?

@Gankra
Copy link
Contributor

Gankra commented Jul 20, 2017

Also it appears that you're trying to make waves have a fixed width, and you just stretch the height. Gecko's algorithm is to have a fixed slope (45 degrees), so the width of a wave increases with its height:

screen shot 2017-07-20 at 3 46 51 pm 1

@glennw
Copy link
Member Author

glennw commented Jul 20, 2017

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

📌 Commit 1fd1eaa has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2017

Testing commit 1fd1eaa with merge 9c75e4a...

bors-servo added a commit that referenced this pull request Jul 20, 2017
Basic implementation of line decoration display items.

Includes the commits in #1491 from @gankro.

This provides a basic implementation of the line decoration styles in CSS.

I haven't specifically tried to make the styles (e.g. wavy, dotted) match other browsers, yet. I think it's reasonable to get this reviewed and merged, and then tweak the visuals as a follow up.

<!-- 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/1504)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing 9c75e4a to master...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@glennw
Copy link
Member Author

glennw commented Jul 21, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

Testing commit 1fd1eaa with merge 8fd6348...

bors-servo added a commit that referenced this pull request Jul 21, 2017
Basic implementation of line decoration display items.

Includes the commits in #1491 from @gankro.

This provides a basic implementation of the line decoration styles in CSS.

I haven't specifically tried to make the styles (e.g. wavy, dotted) match other browsers, yet. I think it's reasonable to get this reviewed and merged, and then tweak the visuals as a follow up.

<!-- 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/1504)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 21, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing 8fd6348 to master...

@bors-servo bors-servo merged commit 1fd1eaa into servo:master Jul 21, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.