Skip to content

/ webrender

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

Create a simpler RadialGradient DisplayItem #996

Closed
wants to merge 5 commits into from
Closed

Create a simpler RadialGradient DisplayItem #996

wants to merge 5 commits into from

Conversation

@eqrion
Copy link
Contributor

eqrion commented Mar 20, 2017 •
edited by larsbergstrom

RadialGradient has more properties in common with SVG radialGradients
than with CSS radial-gradients().

  • CSS gradients need to support ellipse ending shapes, while SVG does not.
  • CSS gradients are used as images and so they need tiling, while SVG does not.
  • SVG gradients need different start and end center's and radii, while CSS does not.

We can simplify the shaders by changing the DisplayItem to be more in line
with CSS radial-gradients(). The existing RadialGradient can then be
renamed to ComplexRadialGradient

This change is Reviewable
@eqrion
Copy link
Contributor Author

eqrion commented Mar 20, 2017

r? @glennw
cc @lsalzman
@mstange
Copy link
Contributor

mstange commented Mar 20, 2017 •
edited

Note that with -moz-element you can use an SVG gradient as a CSS background image which can be tiled. So you might want to have a single display item type that just supports everything.
@eqrion
Copy link
Contributor Author

eqrion commented Mar 20, 2017

So another way of doing this could keep one display item with all the properties for both, and then optimize to a better shader for things like the gradient having the same start and end center.

The reason I didn't go that way was mostly because the display item would need to support either

  • startRadiusXY and endRadiusXY, which makes the gpu data another vec4 larger and the math in the general case more difficult but also never needed because no radial gradients need that much information
  • startRadiusX and endRadiusX and ratioXY (for an ellipse transform), which doesn't seem elegant, but is okay

I am open to ways of keeping one DisplayItem because internally we can optimize for the specific type of gradient.

SVG and -moz-element support also seems to be a long time away.
@nical
Copy link
Collaborator

nical commented Mar 21, 2017

I think that moz-element should be treated generically (render in an intermediate surface and composite it using the image primitive), since it needs to support any kind of content. I don't think we should worry about it for now.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 21, 2017

☔ The latest upstream changes (presumably #999) made this pull request unmergeable. Please resolve the merge conflicts.
@eqrion eqrion force-pushed the eqrion:radial-gradient branch from b496a40 to 7162618 Mar 23, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Mar 27, 2017

☔ The latest upstream changes (presumably #1007) made this pull request unmergeable. Please resolve the merge conflicts.
@eqrion eqrion force-pushed the eqrion:radial-gradient branch from 7162618 to 24b704f Mar 28, 2017
@eqrion
Rename RadialGradient to ComplexRadialGradient
db13899
@eqrion eqrion force-pushed the eqrion:radial-gradient branch from 24b704f to 4339347 Mar 28, 2017
eqrion added 4 commits Mar 18, 2017
@eqrion
Add a RadialGradient display item modeled after CSS gradients
e337f67 
A RadialGradient is designed to support the properties needed for a CSS
radial-gradient(). It has a center, a radius X, a radius Y, a collection
of stops, and a repeat flag. This means it can be an ellipse or a
circle. The prim fragment shader does its calculations on a sphere,
so the prim vertex shader sets up a scale transform for ellipses if
needed.
@eqrion
Switch borders to use radial gradients
43aefaa 
Borders will be better suited to use RadialGradientDisplayItem.
@eqrion
Bump version
dd0a20e
@eqrion
Add radial gradient reftests
Loading status checks…
24cb2d0
@eqrion eqrion force-pushed the eqrion:radial-gradient branch from 4339347 to 24cb2d0 Mar 28, 2017
@glennw
Copy link
Member

glennw commented Mar 28, 2017

Apologies for not getting to this one yet - it looks like the plan is to simplify the items (based on reading the #gfx logs)? If so, should I wait for those changes before looking at this now?
@bors-servo
Copy link
Contributor

bors-servo commented Mar 29, 2017

☔ The latest upstream changes (presumably #1015) made this pull request unmergeable. Please resolve the merge conflicts.
@eqrion
Copy link
Contributor Author

eqrion commented Mar 29, 2017

@glennw Yes, the new plan is to not split the RadialGradient display item. That should be simpler and easier to maintain. I'll close this PR and open a new one for the new branch of work.
@eqrion eqrion closed this Mar 29, 2017
@eqrion eqrion deleted the eqrion:radial-gradient branch Apr 6, 2017
@lsalzman lsalzman mentioned this pull request Feb 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@eqrion @mstange @nical @bors-servo @glennw
You can’t perform that action at this time.