Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3929)

Issue 25484006: Add perspective support to SkMatrix44 initializers. (Closed)

Created:
5 years, 11 months ago by aelias_OOO_until_Jul13
Modified:
5 years, 11 months ago
Reviewers:
caryclark, reed1
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Add perspective support to SkMatrix44 initializers. I noticed SkMatrix <-> SkMatrix44 conversions were dropping the perspective values on the floor. As we use SkMatrix44 heavily in Chromium, I'm concerned this missing code will cause a bug eventually. It should be correct to simply use the bottom row of the 4x4 matrix excluding the third column. Previously committed and reverted, second attempt with fix for incorrect use of SkMScalar/SkScalar. BUG= Committed: http://code.google.com/p/skia/source/detail?r=11624

Patch Set 1 #

Patch Set 2 : Add test and header comments #

Total comments: 2

Patch Set 3 : Fix 3x3 comment #

Patch Set 4 : Rebase #

Patch Set 5 : Reupload for trybots #

Patch Set 6 : Switch SkMatrix inputs to SkScalar and be more careful about scalar comparisons #

Unified diffs Side-by-side diffs Delta from patch set Stats (+71 lines, -11 lines) Patch
M include/utils/SkMatrix44.h View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M src/utils/SkMatrix44.cpp View 1 2 3 4 chunks +7 lines, -8 lines 0 comments Download
M tests/Matrix44Test.cpp View 1 2 3 4 5 5 chunks +54 lines, -3 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
aelias_OOO_until_Jul13
Hi Mike, just cleaning up a loose end I noticed in the code.
5 years, 11 months ago (2013-10-01 21:33:47 UTC) #1
reed1
seems correct. Can we write a test to confirm this?
5 years, 11 months ago (2013-10-02 08:35:20 UTC) #2
aelias_OOO_until_Jul13
Added a test, PTAL.
5 years, 11 months ago (2013-10-02 23:40:16 UTC) #3
reed1
https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h File include/utils/SkMatrix44.h (right): https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h#newcode265 include/utils/SkMatrix44.h:265: * perspective components remain untouched. */ the i.e. section ...
5 years, 11 months ago (2013-10-03 09:00:40 UTC) #4
aelias_OOO_until_Jul13
https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h File include/utils/SkMatrix44.h (right): https://codereview.chromium.org/25484006/diff/5001/include/utils/SkMatrix44.h#newcode265 include/utils/SkMatrix44.h:265: * perspective components remain untouched. */ On 2013/10/03 09:00:40, ...
5 years, 11 months ago (2013-10-03 19:05:36 UTC) #5
reed1
lgtm
5 years, 11 months ago (2013-10-04 08:33:32 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/10001
5 years, 11 months ago (2013-10-04 18:22:01 UTC) #7
commit-bot: I haz the power
Failed to apply patch for src/utils/SkMatrix44.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
5 years, 11 months ago (2013-10-04 18:22:04 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/15001
5 years, 11 months ago (2013-10-04 20:04:41 UTC) #9
commit-bot: I haz the power
Change committed as 11622
5 years, 11 months ago (2013-10-04 20:38:11 UTC) #10
aelias_OOO_until_Jul13
Sorry, looks like this is causing a build failure on 64-bit bots: http://108.170.217.252:10117/builders/Build-Mac10.6-GCC-x86-Debug/builds/1815/steps/BuildTests/logs/stdio "../tests/Matrix44Test.cpp:491: warning: ...
5 years, 11 months ago (2013-10-04 20:56:30 UTC) #11
caryclark
On 2013/10/04 20:56:30, aelias wrote: > Sorry, looks like this is causing a build failure ...
5 years, 11 months ago (2013-10-04 22:13:21 UTC) #12
aelias_OOO_until_Jul13
Reverted in r11623.
5 years, 11 months ago (2013-10-04 22:27:40 UTC) #13
aelias_OOO_until_Jul13
The problem was using SkMScalar instead of SkScalar in the part of the test that ...
5 years, 11 months ago (2013-10-05 01:06:36 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/aelias@chromium.org/25484006/30001
5 years, 11 months ago (2013-10-05 01:06:59 UTC) #15
commit-bot: I haz the power
5 years, 11 months ago (2013-10-05 01:16:33 UTC) #16
Message was sent while issue was closed.
Change committed as 11624

Powered by Google App Engine
This is Rietveld 408576698