Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upNicer shader management #2505
Nicer shader management #2505
Conversation
|
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b1c1e9084a9208f925f4c288b69a6576dd4ed4 Also, isolation of the shaders could help with #2436 in the future. |
|
A lot of orange on the try run. I think the R reftest results are expected, not sure about those Windows failures though? |
|
All windows failures look like WebGL timeouts, which is hardly related. @staktrace any clues? |
|
@kvark I don't know which base revision your try push was made from, but the webgl stuff is green on central. So either you're using an older base rev where stuff was broken, or your patch breaks those tests. I'd try rebasing to latest central to make sure it's not the former; the failures persist then it must be something in your patch. |
|
@staktrace: a try push after rebasing on latest central - https://treeherder.mozilla.org/#/jobs?repo=try&revision=47b5eac4436ebd319bdc0c5f4b7b5dfeb8cc3f90 |
|
Latest try looks good, thanks! @bors-servo r+ |
|
|
Nicer shader management @glennw This is a big (but shallow) refactor of the way we manage shaders: - move all the shader related logic into a separate module. Our `renderer` is still too big - separate getting a lazy shader pointer from its binding - introduce special handling of the `mode` uniform - modernize the shape a bit, including the use of `?` macro The last bit, implemented in 0d6b922, is not essential to the PR but I believe it compliments it somewhat nicely. This commit can be seen as controversial because it makes program binding a little less functional. I'd argue that the device is already stateful (e.g. one is expected to bind a program first, then draw, not the other way around), but can remove the commit if there are objections. The PR reduces the amount of code, separates the concerns better, and makes things nicer to work with overall. The desire to clean it up came to me after messing with the flickering workarounds... I'm deliberately dropping this PR bomb during the weekend for a chance to avoid mergocalypse later on :) <!-- 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/2505) <!-- Reviewable:end -->
|
|
@kvark Seems to be done now. Looking at the timestamps I guess the pending jobs were the talos jobs which run on different hardware. Sometimes there's a backlog. |
kvark commentedMar 10, 2018
•
edited by larsbergstrom
@glennw
This is a big (but shallow) refactor of the way we manage shaders:
rendereris still too bigmodeuniform?macroThe last bit, implemented in 0d6b922, is not essential to the PR but I believe it compliments it somewhat nicely. This commit can be seen as controversial because it makes program binding a little less functional. I'd argue that the device is already stateful (e.g. one is expected to bind a program first, then draw, not the other way around), but can remove the commit if there are objections.
The PR reduces the amount of code, separates the concerns better, and makes things nicer to work with overall. The desire to clean it up came to me after messing with the flickering workarounds...
I'm deliberately dropping this PR bomb during the weekend for a chance to avoid mergocalypse later on :)
This change is