Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127233002:
Added the framework for having canvas/recorder/picture record depth_set's. (Closed)
|
Created:
4 years, 5 months ago by vjiaoblack Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdded the framework for having canvas/recorder/picture record depth_set's.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002
Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518
Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c
Committed: https://skia.googlesource.com/skia/+/e5de130788c8637d2f7df9ddb0241b78e04d5882
Patch Set 1 #
Total comments: 10
Patch Set 2 : Updated comments. #
Total comments: 4
Patch Set 3 : Made requested changes! #
Total comments: 6
Patch Set 4 : Updated requested changes (more functions, added to Debug, etc) #
Total comments: 14
Patch Set 5 : Made requested changes. #Patch Set 6 : Created tests for canvas depth setting #
Total comments: 16
Patch Set 7 : Fixed crumbs #
Total comments: 18
Patch Set 8 : Fixed more crumbs #
Total comments: 2
Patch Set 9 : Fixed more crumbs!! #Patch Set 10 : Added comment and default value to depth #
Total comments: 8
Patch Set 11 : Made requested changes. #Patch Set 12 : Fixed up some small things. #Patch Set 13 : Made setZ translateZ #
Total comments: 21
Patch Set 14 : Made requested changes - most of 'em #Patch Set 15 : Removed some dumb + 33 stuff #Patch Set 16 : Made 3.14 a 3.14f for windows #
Total comments: 2
Patch Set 17 : Added TRANSLATE_Z handler in SkPicturePlayback::handleOp #
Total comments: 2
Patch Set 18 : Added suggested comment #
Total comments: 2
Patch Set 19 : Made requested comment change. #Patch Set 20 : Added recordTranslateZ(); fixed enum declaration placement issue #Patch Set 21 : updated tests to be more rigorous about translateZ in playback #
Messages
Total messages: 93 (34 generated)
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. Added the framework for having canvas / recorder / picture record depth sets. Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows fixed codez Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows Blarg GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2113103002 patch from issue 2113103002 at patchset 1 (http://crrev.com/2113103002#ps1) BUG=skia: ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. Added the framework for having canvas / recorder / picture record depth sets. Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows fixed codez Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows Blarg GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2113103002 patch from issue 2113103002 at patchset 1 (http://crrev.com/2113103002#ps1) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com, robertphillips@google.com
Same as the previous CL, but without the half-baked GM!
Some comments. I would also edit the description to clean up your commit comments -- those will be submitted to git otherwise. https://codereview.chromium.org/2127233002/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/1/include/core/SkCanvas.h#new... include/core/SkCanvas.h:1399: uint32_t curDrawDepth = 0; Member variables need to start with 'f', so this should be fCurDrawDepth. https://codereview.chromium.org/2127233002/diff/1/include/core/SkLights.h File include/core/SkLights.h (right): https://codereview.chromium.org/2127233002/diff/1/include/core/SkLights.h#new... include/core/SkLights.h:2: /* The changes in this file seem unrelated to the description. https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h File include/private/SkRecords.h (right): https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h... include/private/SkRecords.h:83: // setZ is Victor Remove this comment? https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h... include/private/SkRecords.h:223: // TEST_VICTOR Remove this comment? https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h... include/private/SkRecords.h:257: //uint8_t zLevel); Is this needed? https://codereview.chromium.org/2127233002/diff/1/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/1/src/core/SkCanvas.cpp#newco... src/core/SkCanvas.cpp:1524: void SkCanvas::setZ(uint32_t z) { This and getZ() could be inlined directly in the SkCanvas class declaration in SkCanvas.h https://codereview.chromium.org/2127233002/diff/1/src/core/SkLightingShader.cpp File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2127233002/diff/1/src/core/SkLightingShader.c... src/core/SkLightingShader.cpp:1: /* The changes in this file seem unrelated to the description.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. Added the framework for having canvas / recorder / picture record depth sets. Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows fixed codez Merge branch 'master' of https://skia.googlesource.com/skia into correct-shadows Blarg GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2113103002 patch from issue 2113103002 at patchset 1 (http://crrev.com/2113103002#ps1) BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. ==========
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 ==========
https://codereview.chromium.org/2127233002/diff/1/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/1/include/core/SkCanvas.h#new... include/core/SkCanvas.h:1399: uint32_t curDrawDepth = 0; On 2016/07/07 18:43:02, jvanverth1 wrote: > Member variables need to start with 'f', so this should be fCurDrawDepth. Done. https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h File include/private/SkRecords.h (right): https://codereview.chromium.org/2127233002/diff/1/include/private/SkRecords.h... include/private/SkRecords.h:83: // setZ is Victor On 2016/07/07 18:43:02, jvanverth1 wrote: > Remove this comment? Done. https://codereview.chromium.org/2127233002/diff/1/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/1/src/core/SkCanvas.cpp#newco... src/core/SkCanvas.cpp:1524: void SkCanvas::setZ(uint32_t z) { On 2016/07/07 18:43:02, jvanverth1 wrote: > This and getZ() could be inlined directly in the SkCanvas class declaration in > SkCanvas.h Done.
Last comments from me. I defer to robertphillips@ for the rest. https://codereview.chromium.org/2127233002/diff/20001/include/core/SkLights.h File include/core/SkLights.h (left): https://codereview.chromium.org/2127233002/diff/20001/include/core/SkLights.h... include/core/SkLights.h:76: const Light& light(int index) const { Should probably make these const again. https://codereview.chromium.org/2127233002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2127233002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:179: Nit: if you remove this line, then this file doesn't need to be patched.
Done! Sorry for all these code crumbs. I was passively relying on CLion to tell me what I needed to clean, and it wasn't enough :P (or maybe I was doing it wrong) https://codereview.chromium.org/2127233002/diff/20001/include/core/SkLights.h File include/core/SkLights.h (left): https://codereview.chromium.org/2127233002/diff/20001/include/core/SkLights.h... include/core/SkLights.h:76: const Light& light(int index) const { On 2016/07/07 19:08:35, jvanverth1 wrote: > Should probably make these const again. Done. https://codereview.chromium.org/2127233002/diff/20001/src/core/SkLightingShad... File src/core/SkLightingShader.cpp (right): https://codereview.chromium.org/2127233002/diff/20001/src/core/SkLightingShad... src/core/SkLightingShader.cpp:179: On 2016/07/07 19:08:35, jvanverth1 wrote: > Nit: if you remove this line, then this file doesn't need to be patched. Done.
brianosman@google.com changed reviewers: + brianosman@google.com
https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:451: void setZ(uint32_t z) { If this (and the member variable, getter, etc...) are uint8_t, the user can't violate the contract in the comment.
We also want the setZ calls to appear in the debugger! https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:450: */ I don't think Z can be restricted to the index range. We want to treat the Z values as if they were real and applied matrices to them and have the shadows look right. I think we need to remap the Z values to the 0..255 range internally. https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:1401: private: I think we want the Z to be a member of the MCRec class and participate in the save/restore system. I will be up to the use to ensure that the painter's order matches the Z-order.
Implemented changes. Have not yet done the assert to ensure painter's order - thinking I should put it in setZ (only ever allow Z to increase) does that sound good? https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:450: */ On 2016/07/07 20:43:08, robertphillips wrote: > I don't think Z can be restricted to the index range. We want to treat the Z > values as if they were real and applied matrices to them and have the shadows > look right. I think we need to remap the Z values to the 0..255 range > internally. Done. https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:451: void setZ(uint32_t z) { On 2016/07/07 20:06:37, Brian Osman wrote: > If this (and the member variable, getter, etc...) are uint8_t, the user can't > violate the contract in the comment. Acknowledged. https://codereview.chromium.org/2127233002/diff/40001/include/core/SkCanvas.h... include/core/SkCanvas.h:1401: private: On 2016/07/07 20:43:08, robertphillips wrote: > I think we want the Z to be a member of the MCRec class and participate in the > save/restore system. > > I will be up to the use to ensure that the painter's order matches the Z-order. Done.
> Have not yet done the assert to ensure painter's order - > thinking I should put it in setZ (only ever allow Z to increase) does that sound > good? I don't think we actually want that assert anymore.
On 2016/07/08 17:22:24, robertphillips wrote: > > Have not yet done the assert to ensure painter's order - > > thinking I should put it in setZ (only ever allow Z to increase) does that > sound > > good? > > I don't think we actually want that assert anymore. Okay, good to hear. Any more crumbs to sweep up?
Please add a unit test https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:447: /** Set the current draw depth of the canvas. Fix this comment https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:449: 0 is lowest (into screen), 255 is highest (out of screen) Add info that this plays in the save/restore stack. https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:454: */ Fix return value https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:1319: virtual void didSetMatrix(const SkMatrix&) {} don't need const on a scalar https://codereview.chromium.org/2127233002/diff/60001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1530: not uint32_t https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... tools/debugger/SkDrawCommand.cpp:30: #define SKDEBUGCANVAS_ATTRIBUTE_MATRIX "matrix" line up https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... File tools/debugger/SkDrawCommand.h (right): https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... tools/debugger/SkDrawCommand.h:715: public: no const
And added a test! https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:447: /** Set the current draw depth of the canvas. On 2016/07/08 17:26:43, robertphillips wrote: > Fix this comment Done. https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:449: 0 is lowest (into screen), 255 is highest (out of screen) On 2016/07/08 17:26:43, robertphillips wrote: > Add info that this plays in the save/restore stack. Done. https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:454: */ On 2016/07/08 17:26:43, robertphillips wrote: > Fix return value Done. https://codereview.chromium.org/2127233002/diff/60001/include/core/SkCanvas.h... include/core/SkCanvas.h:1319: virtual void didSetMatrix(const SkMatrix&) {} On 2016/07/08 17:26:42, robertphillips wrote: > don't need const on a scalar Done. https://codereview.chromium.org/2127233002/diff/60001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/60001/src/core/SkCanvas.cpp#n... src/core/SkCanvas.cpp:1530: On 2016/07/08 17:26:43, robertphillips wrote: > not uint32_t Done. https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... tools/debugger/SkDrawCommand.cpp:30: #define SKDEBUGCANVAS_ATTRIBUTE_MATRIX "matrix" On 2016/07/08 17:26:43, robertphillips wrote: > line up Done. https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... File tools/debugger/SkDrawCommand.h (right): https://codereview.chromium.org/2127233002/diff/60001/tools/debugger/SkDrawCo... tools/debugger/SkDrawCommand.h:715: public: On 2016/07/08 17:26:43, robertphillips wrote: > no const Done.
https://codereview.chromium.org/2127233002/diff/100001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/100001/include/core/SkCanvas.... include/core/SkCanvas.h:452: void setZ(SkScalar z); Based on today's discussion, this should be changed to translateZ, and its behavior changed to be relative to any previous translateZs. I.e., translateZ(1.0) followed by translateZ(2.0) should end up with a translation of 3.0. I wouldn't do that in this patch, but in a subsequent one. Just noting it for later.
https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: kCanvas* ? https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:787: SkDrawDepthTester(skiatest::Reporter* reporter) { It doesn't look like you are saving off the reporter here https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:794: private: Member variables are prefixed with 'f' not '_' https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:796: The SkPictureRecorder should just be on the stack in test_updatedepth https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:797: SkPictureRecorder _recorder; Why do you need to store _canvas & _picture ? https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:799: SkCanvas* _canvas; Do you use paint at all ? https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:857: } What's with all these lines ?
https://codereview.chromium.org/2127233002/diff/100001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/100001/include/core/SkCanvas.... include/core/SkCanvas.h:452: void setZ(SkScalar z); On 2016/07/11 18:13:43, jvanverth1 wrote: > Based on today's discussion, this should be changed to translateZ, and its > behavior changed to be relative to any previous translateZs. I.e., > translateZ(1.0) followed by translateZ(2.0) should end up with a translation of > 3.0. > > I wouldn't do that in this patch, but in a subsequent one. Just noting it for > later. Acknowledged. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: On 2016/07/11 18:39:35, robertphillips wrote: > kCanvas* ? Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:787: SkDrawDepthTester(skiatest::Reporter* reporter) { On 2016/07/11 18:39:35, robertphillips wrote: > It doesn't look like you are saving off the reporter here Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:794: private: On 2016/07/11 18:39:35, robertphillips wrote: > Member variables are prefixed with 'f' not '_' Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:796: On 2016/07/11 18:39:35, robertphillips wrote: > The SkPictureRecorder should just be on the stack in test_updatedepth Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:797: SkPictureRecorder _recorder; On 2016/07/11 18:39:35, robertphillips wrote: > Why do you need to store _canvas & _picture ? Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:799: SkCanvas* _canvas; On 2016/07/11 18:39:35, robertphillips wrote: > Do you use paint at all ? Done. https://codereview.chromium.org/2127233002/diff/100001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:857: } On 2016/07/11 18:39:35, robertphillips wrote: > What's with all these lines ? Done.
y su https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecordDraw.... src/core/SkRecordDraw.cpp:291: line the '{' up with above ones ? https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecorder.cpp File src/core/SkRecorder.cpp (right): https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecorder.cp... src/core/SkRecorder.cpp:361: don't need const https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecorder.h File src/core/SkRecorder.h (right): https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecorder.h#... src/core/SkRecorder.h:62: void didSetMatrix(const SkMatrix&) override; don't need const https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:781: My suggestion here is to ditch the SkCanvasDrawDepthTester and just call test_updatedepth passing in the reporter. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: kCanvasWidth & kCanvasHeight Also, these don't appear to be used anywhere. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:787: SkCanvasDrawDepthTester(skiatest::Reporter* reporter) { Don't need "this->" here https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:791: void runTests() { testUpdateDepth. The '_' form is for static local methods. Also, since testUpdateDepth isn't static you don't need to pass in fReporter. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:798: void test_updatedepth(skiatest::Reporter* reporter) { I don't know what recording into a picture is giving you in this test. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:803: You could use kCanvasWidth & kCanvasHeight here instead of 100, 100 https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:806: canvas->setZ(-10); REPORTER_ASSERT(reporter, canvas->getZ() == -10);
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/2127233002/diff/140001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/140001/include/core/SkCanvas.... include/core/SkCanvas.h:447: /** Set the current draw depth of the canvas. Is the default 0 or 1?
https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecordDraw.cpp File src/core/SkRecordDraw.cpp (right): https://codereview.chromium.org/2127233002/diff/120001/src/core/SkRecordDraw.... src/core/SkRecordDraw.cpp:291: On 2016/07/11 19:24:52, robertphillips wrote: > line the '{' up with above ones ? Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:781: On 2016/07/11 19:24:52, robertphillips wrote: > My suggestion here is to ditch the SkCanvasDrawDepthTester and just call > test_updatedepth passing in the reporter. Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: On 2016/07/11 19:24:53, robertphillips wrote: > kCanvasWidth & kCanvasHeight > > Also, these don't appear to be used anywhere. Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:787: SkCanvasDrawDepthTester(skiatest::Reporter* reporter) { On 2016/07/11 19:24:53, robertphillips wrote: > Don't need "this->" here Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:791: void runTests() { On 2016/07/11 19:24:53, robertphillips wrote: > testUpdateDepth. The '_' form is for static local methods. > > Also, since testUpdateDepth isn't static you don't need to pass in fReporter. Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:798: void test_updatedepth(skiatest::Reporter* reporter) { On 2016/07/11 19:24:52, robertphillips wrote: > I don't know what recording into a picture is giving you in this test. Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:803: On 2016/07/11 19:24:52, robertphillips wrote: > You could use kCanvasWidth & kCanvasHeight here instead of 100, 100 Done. https://codereview.chromium.org/2127233002/diff/120001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:806: canvas->setZ(-10); On 2016/07/11 19:24:53, robertphillips wrote: > REPORTER_ASSERT(reporter, canvas->getZ() == -10); Done. https://codereview.chromium.org/2127233002/diff/140001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/140001/include/core/SkCanvas.... include/core/SkCanvas.h:447: /** Set the current draw depth of the canvas. On 2016/07/11 19:36:28, reed1 wrote: > Is the default 0 or 1? Done.
lgtm https://codereview.chromium.org/2127233002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:296: Should probably do this in the ctor with the others. https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:782: static https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:786: SkCanvas* canvas; picture isn't used https://codereview.chromium.org/2127233002/diff/180001/tools/debugger/SkDrawC... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3258: SkSetZCommand* SkSetZCommand::fromJSON(Json::Value& command, line this up with the prior parameter ?
Okay. Committing ~ https://codereview.chromium.org/2127233002/diff/180001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:296: On 2016/07/11 20:57:26, robertphillips wrote: > Should probably do this in the ctor with the others. Done. https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:782: On 2016/07/11 20:57:26, robertphillips wrote: > static Done. https://codereview.chromium.org/2127233002/diff/180001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:786: SkCanvas* canvas; On 2016/07/11 20:57:26, robertphillips wrote: > picture isn't used Done. https://codereview.chromium.org/2127233002/diff/180001/tools/debugger/SkDrawC... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/180001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3258: SkSetZCommand* SkSetZCommand::fromJSON(Json::Value& command, On 2016/07/11 20:57:27, robertphillips wrote: > line this up with the prior parameter ? Done.
Okay. Committing ~
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2127233002/#ps200001 (title: "Made requested changes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
vjiaoblack@google.com changed reviewers: + bsalomon@google.com
What do people think of translateZ rather than setZ? It seems to me like a hierarchical view system may want a push/pop/translate interface rather than set. We've worked hard to remove sets from the API for clips and matrices.
On 2016/07/12 13:33:54, bsalomon wrote: > What do people think of translateZ rather than setZ? It seems to me like a > hierarchical view system may want a push/pop/translate interface rather than > set. We've worked hard to remove sets from the API for clips and matrices. So then, getZ (or peekZ), pushZ, popZ? I guess what you're saying does make sense, and it would work with the existing system better. I think I'd have to make a couple more changes to Records and stuff, right?
On 2016/07/12 13:33:54, bsalomon wrote: > What do people think of translateZ rather than setZ? It seems to me like a > hierarchical view system may want a push/pop/translate interface rather than > set. We've worked hard to remove sets from the API for clips and matrices. I see Jim's earlier comment now. Why not do it in this patch? Also, what's the intended use of getZ? We generally don't add state getters on SkCanvas because it can be misleading to clients. When the modifiers are relative rather than absolute (translate vs set) then the actual z value of the draw in the case of SkPicture will depend on the context in which the picture is played back, hence we've been removing getters for matrix and clip state over time.
On 2016/07/12 13:39:02, vjiaoblack wrote: > On 2016/07/12 13:33:54, bsalomon wrote: > > What do people think of translateZ rather than setZ? It seems to me like a > > hierarchical view system may want a push/pop/translate interface rather than > > set. We've worked hard to remove sets from the API for clips and matrices. > > So then, > getZ (or peekZ), pushZ, popZ? I wouldn't have getZ() (see my other comment). Does this need its own push/pop API or can it work with the existing save/restore calls? > > I guess what you're saying does make sense, and it would work with the existing > system better. > > I think I'd have to make a couple more changes to Records and stuff, right?
On 2016/07/12 13:39:32, bsalomon wrote: > On 2016/07/12 13:33:54, bsalomon wrote: > > What do people think of translateZ rather than setZ? It seems to me like a > > hierarchical view system may want a push/pop/translate interface rather than > > set. We've worked hard to remove sets from the API for clips and matrices. > > I see Jim's earlier comment now. Why not do it in this patch? > > Also, what's the intended use of getZ? We generally don't add state getters on > SkCanvas because it can be misleading to clients. When the modifiers are > relative rather than absolute (translate vs set) then the actual z value of the > draw in the case of SkPicture will depend on the context in which the picture is > played back, hence we've been removing getters for matrix and clip state over > time. I think I need it to put in the onFilter() for rendering Z values to an implemented SkPaintFilterCanvas, to actually generate the shadow map.
On 2016/07/12 13:41:15, vjiaoblack wrote: > On 2016/07/12 13:39:32, bsalomon wrote: > > On 2016/07/12 13:33:54, bsalomon wrote: > > > What do people think of translateZ rather than setZ? It seems to me like a > > > hierarchical view system may want a push/pop/translate interface rather > than > > > set. We've worked hard to remove sets from the API for clips and matrices. > > > > I see Jim's earlier comment now. Why not do it in this patch? > > > > Also, what's the intended use of getZ? We generally don't add state getters on > > SkCanvas because it can be misleading to clients. When the modifiers are > > relative rather than absolute (translate vs set) then the actual z value of > the > > draw in the case of SkPicture will depend on the context in which the picture > is > > played back, hence we've been removing getters for matrix and clip state over > > time. > > I think I need it to put in the onFilter() for rendering Z values to > an implemented SkPaintFilterCanvas, to actually generate the shadow map. SkPaintFilterCanvas is a SkCanvas subclass, so it could be a protected getter.
On 2016/07/12 13:49:01, bsalomon wrote: > On 2016/07/12 13:41:15, vjiaoblack wrote: > > On 2016/07/12 13:39:32, bsalomon wrote: > > > On 2016/07/12 13:33:54, bsalomon wrote: > > > > What do people think of translateZ rather than setZ? It seems to me like a > > > > hierarchical view system may want a push/pop/translate interface rather > > than > > > > set. We've worked hard to remove sets from the API for clips and matrices. > > > > > > I see Jim's earlier comment now. Why not do it in this patch? > > > > > > Also, what's the intended use of getZ? We generally don't add state getters > on > > > SkCanvas because it can be misleading to clients. When the modifiers are > > > relative rather than absolute (translate vs set) then the actual z value of > > the > > > draw in the case of SkPicture will depend on the context in which the > picture > > is > > > played back, hence we've been removing getters for matrix and clip state > over > > > time. > > > > I think I need it to put in the onFilter() for rendering Z values to > > an implemented SkPaintFilterCanvas, to actually generate the shadow map. > > SkPaintFilterCanvas is a SkCanvas subclass, so it could be a protected getter. Okay. Shall I attempt to make this change?
On 2016/07/12 13:49:37, vjiaoblack wrote: > On 2016/07/12 13:49:01, bsalomon wrote: > > On 2016/07/12 13:41:15, vjiaoblack wrote: > > > On 2016/07/12 13:39:32, bsalomon wrote: > > > > On 2016/07/12 13:33:54, bsalomon wrote: > > > > > What do people think of translateZ rather than setZ? It seems to me like > a > > > > > hierarchical view system may want a push/pop/translate interface rather > > > than > > > > > set. We've worked hard to remove sets from the API for clips and > matrices. > > > > > > > > I see Jim's earlier comment now. Why not do it in this patch? > > > > > > > > Also, what's the intended use of getZ? We generally don't add state > getters > > on > > > > SkCanvas because it can be misleading to clients. When the modifiers are > > > > relative rather than absolute (translate vs set) then the actual z value > of > > > the > > > > draw in the case of SkPicture will depend on the context in which the > > picture > > > is > > > > played back, hence we've been removing getters for matrix and clip state > > over > > > > time. > > > > > > I think I need it to put in the onFilter() for rendering Z values to > > > an implemented SkPaintFilterCanvas, to actually generate the shadow map. > > > > SkPaintFilterCanvas is a SkCanvas subclass, so it could be a protected getter. > > Okay. > > Shall I attempt to make this change? IMO yes, unless there is a rush to land it as is.
https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.... include/core/SkCanvas.h:454: protected: We usually try to just have one public, protected and private section in each class. Could move this below? https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3244: fZTranslate += z + 33; What's the += and 33 about?
https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.... include/core/SkCanvas.h:454: protected: add the word "cumulative" somewhere here ? https://codereview.chromium.org/2127233002/diff/240001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:296: // This is the current cumulative depth (aggregating the individual translateZ calls) ? https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:781: This is kind of strange ... https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: If you want this to be a member function it should be named testUpdateDepth https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:784: void test_updatedepth(skiatest::Reporter* reporter) { This comment shouldn't mention picture anymore https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:786: If you want this to be a member function all the canvas methods need to be preceeded with "this->" https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:836: Can you not just do: { SkCanvas testCanvas(100, 100); test_updatedepth(reporter, &testCanvas); } and skip all the picture stuff and deriving an SkTestCanvas? https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3248: void SkTranslateZCommand::execute(SkCanvas* canvas) const { same here https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3261: extract_json_scalar(command[SKDEBUGCANVAS_ATTRIBUTE_DRAWDEPTHTRANS], &z); here too
https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/240001/include/core/SkCanvas.... include/core/SkCanvas.h:454: protected: On 2016/07/12 15:34:32, robertphillips wrote: > add the word "cumulative" somewhere here ? Sure, but I think that's redundant! https://codereview.chromium.org/2127233002/diff/240001/src/core/SkCanvas.cpp File src/core/SkCanvas.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/src/core/SkCanvas.cpp#... src/core/SkCanvas.cpp:296: On 2016/07/12 15:34:32, robertphillips wrote: > // This is the current cumulative depth (aggregating the individual translateZ > calls) > > ? Done. https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp File tests/CanvasTest.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:781: On 2016/07/12 15:34:32, robertphillips wrote: > This is kind of strange ... Since we made getZ a protected method, I now know of no other way to access the Z values to check if they're right with asserts. https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:783: public: On 2016/07/12 15:34:32, robertphillips wrote: > If you want this to be a member function it should be named testUpdateDepth Done. https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:784: void test_updatedepth(skiatest::Reporter* reporter) { On 2016/07/12 15:34:32, robertphillips wrote: > This comment shouldn't mention picture anymore Done. https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:786: On 2016/07/12 15:34:32, robertphillips wrote: > If you want this to be a member function all the canvas methods need to be > preceeded with "this->" Done. https://codereview.chromium.org/2127233002/diff/240001/tests/CanvasTest.cpp#n... tests/CanvasTest.cpp:836: On 2016/07/12 15:34:32, robertphillips wrote: > Can you not just do: > > { > SkCanvas testCanvas(100, 100); > > test_updatedepth(reporter, &testCanvas); > } > > and skip all the picture stuff and deriving an SkTestCanvas? Uh. Oh. pfft, right. I'll do that. *tried to do it* oh right, getZ() is protected. Can only do it from within the class. (?) https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... File tools/debugger/SkDrawCommand.cpp (right): https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3244: fZTranslate += z + 33; On 2016/07/12 15:26:34, bsalomon wrote: > What's the += and 33 about? Done. https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3248: void SkTranslateZCommand::execute(SkCanvas* canvas) const { On 2016/07/12 15:34:32, robertphillips wrote: > same here Done. https://codereview.chromium.org/2127233002/diff/240001/tools/debugger/SkDrawC... tools/debugger/SkDrawCommand.cpp:3261: extract_json_scalar(command[SKDEBUGCANVAS_ATTRIBUTE_DRAWDEPTHTRANS], &z); On 2016/07/12 15:34:32, robertphillips wrote: > here too Done.
lgtm
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2127233002/#ps280001 (title: "Removed some dumb + 33 stuff")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
Fixed the windows bug by specifying 3.14f instead of 3.14
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2127233002/#ps300001 (title: "Made 3.14 a 3.14f for windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2142183003/ by mtklein@google.com. The reason for reverting is: our bots are segfaulting inside the .skp-loading code, so this seems like the likely candidate..
Message was sent while issue was closed.
On 2016/07/12 22:16:54, mtklein wrote: > A revert of this CL (patchset #16 id:300001) has been created in > https://codereview.chromium.org/2142183003/ by mailto:mtklein@google.com. > > The reason for reverting is: our bots are segfaulting inside the .skp-loading > code, so this seems like the likely candidate.. Unknown draw type: 70 ../../../src/core/SkPicturePlayback.cpp:620: fatal error: ""(false) || (SkDebugf(\"Unknown draw type: %d\"\"\\n\", op), false)""
Message was sent while issue was closed.
On 2016/07/12 22:17:36, mtklein wrote: > On 2016/07/12 22:16:54, mtklein wrote: > > A revert of this CL (patchset #16 id:300001) has been created in > > https://codereview.chromium.org/2142183003/ by mailto:mtklein@google.com. > > > > The reason for reverting is: our bots are segfaulting inside the .skp-loading > > code, so this seems like the likely candidate.. > > Unknown draw type: 70 > ../../../src/core/SkPicturePlayback.cpp:620: fatal error: ""(false) || > (SkDebugf(\"Unknown draw type: %d\"\"\\n\", op), false)"" Or possibly, ../../src/core/SkPicturePlayback.cpp:137: fatal error: ""!offsetToRestore || offsetToRestore >= reader->offset()""
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2142773004/ by msarett@google.com. The reason for reverting is: I believe that this is causing crashes in nanobench: https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX... ../../../src/core/SkReader32.h:59: fatal error: ""fCurr <= fStop"" Signal 6 [Aborted]: /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench[0xbb79e3] /lib/x86_64-linux-gnu/libc.so.6(+0x36d40)[0x7ff2c9250d40] /lib/x86_64-linux-gnu/libc.so.6(gsignal+0x39)[0x7ff2c9250cc9] /lib/x86_64-linux-gnu/libc.so.6(abort+0x148)[0x7ff2c92540d8] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_Z16sk_out_of_memoryv+0x0)[0x10e80f1] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN10SkReader327readIntEv+0x9a)[0xd63c06] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN12SkReadBuffer7readIntEv+0x1c)[0xd628e8] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZNK13SkPictureData10getPictureEP12SkReadBuffer+0x27)[0xd505eb] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN17SkPicturePlayback8handleOpEP12SkReadBuffer8DrawTypejP8SkCanvasRK8SkMatrix+0x15ee)[0xd4ed02] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN17SkPicturePlayback4drawEP8SkCanvasPN9SkPicture13AbortCallbackEPK12SkReadBuffer+0x2a4)[0xd4d694] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN9SkPicture11ForwardportERK10SkPictInfoPK13SkPictureDataPK12SkReadBuffer+0xc7)[0xd462c9] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN9SkPicture14MakeFromStreamEP8SkStreamPFbPKvmP8SkBitmapEP18SkTypefacePlayback+0xc6)[0xd464ce] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN13SkPictureData14parseStreamTagEP8SkStreamjjPFbPKvmP8SkBitmapEP18SkTypefacePlayback+0x49d)[0xd494df] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN13SkPictureData11parseStreamEP8SkStreamPFbPKvmP8SkBitmapEP18SkTypefacePlayback+0x6b)[0xd49ea7] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN13SkPictureData16CreateFromStreamEP8SkStreamRK10SkPictInfoPFbPKvmP8SkBitmapEP18SkTypefacePlayback+0x81)[0xd49d67] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN9SkPicture14MakeFromStreamEP8SkStreamPFbPKvmP8SkBitmapEP18SkTypefacePlayback+0x90)[0xd46498] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN9SkPicture14MakeFromStreamEP8SkStream+0x2a)[0xd463ce] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN15BenchmarkStream11ReadPictureEPKc+0xe0)[0xa69868] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN15BenchmarkStream7rawNextEv+0x20e)[0xa69b86] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_ZN15BenchmarkStream4nextEv+0x2a)[0xa698c4] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(_Z14nanobench_mainv+0x4d7)[0xa669da] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench(main+0x25)[0xa67962] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5)[0x7ff2c923bec5] /b/swarm_slave/work/isolated/isolated_runNd_Y6X/out/Debug/nanobench[0x9d3b79] step returned non-zero exit code: 6 .
Message was sent while issue was closed.
https://codereview.chromium.org/2127233002/diff/300001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/300001/include/core/SkCanvas.... include/core/SkCanvas.h:452: void translateZ(SkScalar z); As documented, this "sets" the z value. That differs from what our other methods do: translate/scale/rotate all modify the existing value w/ their operation, not set it. Lets chat in-person about the API naming
Message was sent while issue was closed.
https://codereview.chromium.org/2127233002/diff/300001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/300001/include/core/SkCanvas.... include/core/SkCanvas.h:452: void translateZ(SkScalar z); On 2016/07/12 22:58:03, reed1 wrote: > As documented, this "sets" the z value. That differs from what our other methods > do: translate/scale/rotate all modify the existing value w/ their operation, not > set it. > > Lets chat in-person about the API naming The method name is correct; the comment is deceptive and needs to be updated.
Message was sent while issue was closed.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 ==========
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
The CQ bit was unchecked by vjiaoblack@google.com
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2127233002/diff/320001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/320001/include/core/SkCanvas.... include/core/SkCanvas.h:447: /** Change the current draw depth of the canvas with a translation. Suggestion: Add the specified translation to the current draw depth of the canvas. @param z The distance to translate in z. Negative into screen, positive out of screen. Without translation, the draw depth defaults to 0.
https://codereview.chromium.org/2127233002/diff/340001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/340001/include/core/SkCanvas.... include/core/SkCanvas.h:448: @param z The SkScalar depth; it's tracked in the save/restore stack. I would change this to something like I suggested -- it matches the matrix translate() function.
https://codereview.chromium.org/2127233002/diff/320001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/320001/include/core/SkCanvas.... include/core/SkCanvas.h:447: /** Change the current draw depth of the canvas with a translation. On 2016/07/13 13:54:24, jvanverth1 wrote: > Suggestion: Add the specified translation to the current draw depth of the > canvas. > @param z The distance to translate in z. > Negative into screen, positive out of screen. > > Without translation, the draw depth defaults to 0. Done. https://codereview.chromium.org/2127233002/diff/340001/include/core/SkCanvas.h File include/core/SkCanvas.h (right): https://codereview.chromium.org/2127233002/diff/340001/include/core/SkCanvas.... include/core/SkCanvas.h:448: @param z The SkScalar depth; it's tracked in the save/restore stack. On 2016/07/13 14:39:25, jvanverth1 wrote: > I would change this to something like I suggested -- it matches the matrix > translate() function. Done.
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2127233002/#ps360001 (title: "Made requested comment change.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:360001) has been created in https://codereview.chromium.org/2150573002/ by mtklein@chromium.org. The reason for reverting is: Still crashing....
Message was sent while issue was closed.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c ==========
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vjiaoblack@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, robertphillips@google.com Link to the patchset: https://codereview.chromium.org/2127233002/#ps400001 (title: "updated tests to be more rigorous about translateZ in playback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c ========== to ========== Added the framework for having canvas/recorder/picture record depth_set's. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2127233002 Committed: https://skia.googlesource.com/skia/+/6d3fb898d5f73a82e36f11c712a633c3921ed518 Committed: https://skia.googlesource.com/skia/+/1185d90c785f743364cc9113d7007a59af07470c Committed: https://skia.googlesource.com/skia/+/e5de130788c8637d2f7df9ddb0241b78e04d5882 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://skia.googlesource.com/skia/+/e5de130788c8637d2f7df9ddb0241b78e04d5882
Message was sent while issue was closed.
CQ bit was unchecked. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
