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

revise gradients to use a lookup texture (supporting large amounts of… #792

Merged
merged 1 commit into from Jan 31, 2017

Conversation

Projects
None yet
4 participants
@lsalzman
Contributor

lsalzman commented Jan 27, 2017

… stops cheaply) and allow repeating


This change is Reviewable

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Jan 30, 2017

Member

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


webrender/src/internal_types.rs, line 292 at r1 (raw file):

    }

    pub fn to_bgra(&self) -> PackedColor {

If we do have a BGRA color (see comments below), this should probably be a different type, to make use of the type system.


webrender/src/prim_store.rs, line 275 at r1 (raw file):

// the offset within that entry bucket is used to interpolate between the two colors in that entry.
// This layout preserves hard stops, as the end color for a given entry can differ from the start
// color for the following entry, despite them being adjacent. Colors are stored within in BGRA8

Is there a specific reason to introduce the BGRA code? It seems to introduce some additional complexity (esp. if PackedColor can be either format).


webrender/src/prim_store.rs, line 289 at r1 (raw file):

}

impl Clone for GradientData {

I don't think there's any need to implement this - it can be derived if required with #[derive(Clone)]


webrender/src/prim_store.rs, line 728 at r1 (raw file):

                let metadata = PrimitiveMetadata {
                    is_opaque: false,

Let's add a TODO comment here to calculate if the primitive is actually opaque (so that it can be renderer in the opaque pass).


webrender/src/renderer.rs, line 149 at r1 (raw file):

    fn texture_width() -> usize {
        2 * GRADIENT_DATA_RESOLUTION

We should add a comment here explaining why it's 2*


Comments from Reviewable

Member

glennw commented Jan 30, 2017

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


webrender/src/internal_types.rs, line 292 at r1 (raw file):

    }

    pub fn to_bgra(&self) -> PackedColor {

If we do have a BGRA color (see comments below), this should probably be a different type, to make use of the type system.


webrender/src/prim_store.rs, line 275 at r1 (raw file):

// the offset within that entry bucket is used to interpolate between the two colors in that entry.
// This layout preserves hard stops, as the end color for a given entry can differ from the start
// color for the following entry, despite them being adjacent. Colors are stored within in BGRA8

Is there a specific reason to introduce the BGRA code? It seems to introduce some additional complexity (esp. if PackedColor can be either format).


webrender/src/prim_store.rs, line 289 at r1 (raw file):

}

impl Clone for GradientData {

I don't think there's any need to implement this - it can be derived if required with #[derive(Clone)]


webrender/src/prim_store.rs, line 728 at r1 (raw file):

                let metadata = PrimitiveMetadata {
                    is_opaque: false,

Let's add a TODO comment here to calculate if the primitive is actually opaque (so that it can be renderer in the opaque pass).


webrender/src/renderer.rs, line 149 at r1 (raw file):

    fn texture_width() -> usize {
        2 * GRADIENT_DATA_RESOLUTION

We should add a comment here explaining why it's 2*


Comments from Reviewable

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Jan 30, 2017

Member

This looks great! I left a few comments and questions via reviewable. This will also need a rebase, and it looks like the shader validation is currently failing on CI.

Member

glennw commented Jan 30, 2017

This looks great! I left a few comments and questions via reviewable. This will also need a rebase, and it looks like the shader validation is currently failing on CI.

@kvark

Good stuff! Please address my comments below

Show outdated Hide outdated webrender/src/renderer.rs Outdated
Show outdated Hide outdated webrender/src/tiling.rs Outdated
Show outdated Hide outdated webrender/src/prim_store.rs Outdated
Show outdated Hide outdated webrender/src/prim_store.rs Outdated
Show outdated Hide outdated webrender/src/prim_store.rs Outdated
Show outdated Hide outdated webrender/res/ps_angle_gradient.fs.glsl Outdated
Show outdated Hide outdated webrender/res/ps_radial_gradient.fs.glsl Outdated
Show outdated Hide outdated webrender/src/internal_types.rs Outdated
@kvark

kvark approved these changes Jan 30, 2017

ok, I think this PR is good. The remaining concerns are minor.

Show outdated Hide outdated webrender/res/ps_angle_gradient.fs.glsl Outdated
Show outdated Hide outdated webrender/src/prim_store.rs Outdated
@kvark

kvark approved these changes Jan 30, 2017

Show outdated Hide outdated webrender/res/ps_angle_gradient.fs.glsl Outdated
Show outdated Hide outdated webrender/res/ps_radial_gradient.fs.glsl Outdated
@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Jan 30, 2017

Member

ugh, github review comments are aweful
/me makes a note to switch to using Reviewable

Member

kvark commented Jan 30, 2017

ugh, github review comments are aweful
/me makes a note to switch to using Reviewable

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Jan 31, 2017

Member

This looks good to me. I've run all the CSS tests on Servo. Needs a rebase (and the shader validation fix on CI if that remains), then this should be ready to merge.

Member

glennw commented Jan 31, 2017

This looks good to me. I've run all the CSS tests on Servo. Needs a rebase (and the shader validation fix on CI if that remains), then this should be ready to merge.

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jan 31, 2017

Contributor

☔️ The latest upstream changes (presumably #763) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor

bors-servo commented Jan 31, 2017

☔️ The latest upstream changes (presumably #763) made this pull request unmergeable. Please resolve the merge conflicts.

@glennw

This comment has been minimized.

Show comment
Hide comment
@glennw

glennw Jan 31, 2017

Member

@bors-servo r=glennw,kvark

Member

glennw commented Jan 31, 2017

@bors-servo r=glennw,kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jan 31, 2017

Contributor

📌 Commit 2f3cf30 has been approved by glennw,kvark

Contributor

bors-servo commented Jan 31, 2017

📌 Commit 2f3cf30 has been approved by glennw,kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jan 31, 2017

Contributor

⌛️ Testing commit 2f3cf30 with merge 3ad2511...

Contributor

bors-servo commented Jan 31, 2017

⌛️ Testing commit 2f3cf30 with merge 3ad2511...

bors-servo added a commit that referenced this pull request Jan 31, 2017

Auto merge of #792 - lsalzman:gradient-rewrite, r=glennw,kvark
revise gradients to use a lookup texture (supporting large amounts of…

… stops cheaply) and allow repeating

<!-- 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/792)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Jan 31, 2017

Contributor

☀️ Test successful - status-travis

Contributor

bors-servo commented Jan 31, 2017

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 2f3cf30 into servo:master Jan 31, 2017

3 of 4 checks passed

code-review/reviewable 16 files, 11 discussions left (glennw, kvark, lsalzman)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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