Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16854012:
Update Alligned Rect Shader to match geometry version (Closed)
|
Created:
7 years, 6 months ago by egdaniel Modified:
7 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionUpdate Alligned Rect Shader to match geometry version
-Also update to combine width and height coverage using multiplication
instead of using min
Committed: http://code.google.com/p/skia/source/detail?r=9609
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : Compilation fix #Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/16854012/diff/1/include/gpu/GrAARectRenderer.h File include/gpu/GrAARectRenderer.h (right): https://codereview.chromium.org/16854012/diff/1/include/gpu/GrAARectRenderer.... include/gpu/GrAARectRenderer.h:47: #define SHADER_AA_FILL_RECT 1 Are we ready for this? https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); static const? I'm not sure, but I think windows may (dumbly) give you a double to float truncation warning if you don't make this 0.5f. https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:69: builder->fsCodeAppend("\tfloat scaleW = min(1.0, 2*insetW/(spanW));\n"); need the parens in the divisor?
https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); On 2013/06/13 14:27:51, bsalomon wrote: I will make it const, but is there a static in glsl? The variable is mostly there right now as a place holder for if/when I try different outset widths. Or do you mean define it within the c++ class? > static const? > > I'm not sure, but I think windows may (dumbly) give you a double to float > truncation warning if you don't make this 0.5f.
https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); On 2013/06/13 14:52:18, egdaniel wrote: > On 2013/06/13 14:27:51, bsalomon wrote: > I will make it const, but is there a static in glsl? The variable is mostly > there right now as a place holder for if/when I try different outset widths. Or > do you mean define it within the c++ class? > > static const? > > > > I'm not sure, but I think windows may (dumbly) give you a double to float > > truncation warning if you don't make this 0.5f. > I'm a moron... I was reading this as c code. ignore both comments.
This CL fails on the complexclip2_rect_aa GM. https://codereview.chromium.org/16854012/diff/1/include/gpu/GrAARectRenderer.h File include/gpu/GrAARectRenderer.h (right): https://codereview.chromium.org/16854012/diff/1/include/gpu/GrAARectRenderer.... include/gpu/GrAARectRenderer.h:47: #define SHADER_AA_FILL_RECT 1 Not yet. https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:61: This comment is now orphaned https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); // For rects > 1 pixel wide and tall the span*s are noops (i.e., 1.0). For rects < 1 pixel wide or tall they serve to normalize the < 1 ramp to a 0 .. 1 range. https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:72: // Compute the coverage for the rect's width Why not put the scales inside the clamps?
Also, could you double check that the dashing3 GM has been improved?
I checked it on dashing3, and the "min" version is the same as the geometry version. The multiply version has the same general "imporvement" towards the raster version as we saw in thinrects. Also I believe the test failed on complexclip2_rect_aa before my CL as well (the error about aColor and aAttrib1 being assigned same genericl vertex attribute). https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:61: What do you mean by orphaned? Both the TODO and the comment below it are still valid. On 2013/06/13 17:45:52, robertphillips wrote: > This comment is now orphaned https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); On 2013/06/13 17:45:52, robertphillips wrote: > // For rects > 1 pixel wide and tall the span*s are noops (i.e., 1.0). For rects > < 1 pixel wide or tall they serve to normalize the < 1 ramp to a 0 .. 1 range. I'm sorry I'm confused, are you simply stating what the code does, should be doing, or suggesting I add that as a comment since I do believe the code does what is described https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:72: // Compute the coverage for the rect's width On 2013/06/13 17:45:52, robertphillips wrote: > Why not put the scales inside the clamps? What would this save us or improve? To keep the algorithm the same, if I put the scales inside the clamp then I would also need to set the clamps max bound to be equal to the scale. My general idea of what the scale is doing (which I believe is how geometry is also implemented) is that the ramp at the outset is zero and the ramp at the inset is "scale". And then anything inside of the inset also set to scale. Is this correct?
https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/1/src/gpu/GrAARectRenderer.cpp#... src/gpu/GrAARectRenderer.cpp:61: It isn't as relevant in this iteration but in an earlier version the computation of the scales was further away from the comments. https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:66: builder->fsCodeAppend("\tfloat outset = 0.5;\n"); Add a comment https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:72: // Compute the coverage for the rect's width So, was the inclusion of the scale inside the clamp in the prior version incorrect?
https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/10001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:72: // Compute the coverage for the rect's width On 2013/06/13 19:55:24, robertphillips wrote: > So, was the inclusion of the scale inside the clamp in the prior version > incorrect? So in the previous version, the scale worked slightly differently. In that version the scale was always >= 1. In essence the old scale was performing an equivalent function as the divide by span that I have which is to transform a scale of "0 to x" where x is < 1 to a scale of "0 to 1". The new scale I've added is there as more of a may to cap the maximum coverage.
lgtm + nit https://codereview.chromium.org/16854012/diff/19001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/16854012/diff/19001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:80: // Compute the coverage for the rect's height and merge with the width rm xtra space?
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/egdaniel@google.com/16854012/19001
Message was sent while issue was closed.
Change committed as 9609
Message was sent while issue was closed.
Message was sent while issue was closed.
lgtm |
