Adds basic sprite sheet support to sprites

Adds basic sprite sheet support to sprites

Adds drawImageRect to Canvas bindings

R=eseidel@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/e9d5f500fbb3e575af25d83d291c403149460f05

[viktorl, 2015-06-23 21:47:41]

viktorl@google.com changed reviewers:
+ eseidel@chromium.org

[viktorl, 2015-06-23 21:48:35]

viktorl@google.com changed reviewers:
+ ianh@google.com
- eseidel@chromium.org

[jackson, 2015-06-23 21:56:53]

jackson@google.com changed reviewers:
+ jackson@google.com

[jackson, 2015-06-23 21:56:55]

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
File sky/examples/game/lib/sprite.dart (right):

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:40: else {
I would move this onto the previous line

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:78: if (w <= 0 || h <= 0) return;
I would move return into the next line

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:80: double scaleX = size.width/w;
I would put a space around binary operators like /
https://www.dartlang.org/articles/style-guide/

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:81: double scaleY = size.height/h;
ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:86: canvas.translate(0.0, (size.height -
scaleX * h)/2.0);
ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:90: canvas.translate((size.width - scaleY *
w)/2.0, 0.0);
ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/sprite.dart:108: canvas.drawImageRect(texture.image,
texture._frame, texture._spriteSourceSize /*new Rect.fromLTRB(0.0, 0.0, w, h)*/,
paint);
Did you mean to leave this comment in here? We don't usually use /**/ style
comments

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
File sky/examples/game/lib/spritesheet.dart (right):

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/spritesheet.dart:6: Map<String, Texture> _textures;
You should be able to initialize this here.

[Hixie, 2015-06-23 22:03:49]

High level comments:

Split this into two CLs, one for the back-end changes and one for the Game
changes.

Add an example of this in examples/raw/ that shows how the API works in
isolation.

Add some tests for the new back-end API.

Make sure it's supported in the tests/resources/display_list.dart test tools.

[Hixie, 2015-06-23 22:03:52]

ianh@google.com changed reviewers:
- jackson@google.com

[Hixie, 2015-06-23 22:03:52]

https://codereview.chromium.org/1204783003/diff/1/sky/engine/core/painting/Ca...
File sky/engine/core/painting/Canvas.cpp (right):

https://codereview.chromium.org/1204783003/diff/1/sky/engine/core/painting/Ca...
sky/engine/core/painting/Canvas.cpp:224: ASSERT(image);
assert that paint is not null (also should happen in drawImage()).

[jackson, 2015-06-23 22:09:42]

jackson@google.com changed reviewers:
+ jackson@google.com

[jackson, 2015-06-23 22:09:43]

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
File sky/examples/game/lib/spritesheet.dart (right):

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/sprit...
sky/examples/game/lib/spritesheet.dart:1: part of sprites;
check indentation here

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
File sky/examples/game/lib/texture.dart (right):

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:5: Size _size;
You might want to make this public and final instead of private and having a
public getter

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:6: String _fileName;
Ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:7: bool _rotated;
final?

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:8: bool _trimmed;
final?

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:10: Rect _frame;
Ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:11: Rect _spriteSourceSize;
Ditto

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:13: Point _pivot;
final?

https://codereview.chromium.org/1204783003/diff/1/sky/examples/game/lib/textu...
sky/examples/game/lib/texture.dart:16: _size = new Size(image.width.toDouble(),
image.height.toDouble());
Usually it's good to initialize members after the : and before the {} if you can

[eseidel, 2015-06-23 23:22:30]

eseidel@chromium.org changed reviewers:
+ eseidel@chromium.org

[eseidel, 2015-06-23 23:22:32]

A few more nits.

https://codereview.chromium.org/1204783003/diff/20001/mojo/dart/mojom/test/pa...
File mojo/dart/mojom/test/packages (right):

https://codereview.chromium.org/1204783003/diff/20001/mojo/dart/mojom/test/pa...
mojo/dart/mojom/test/packages:1: ../packages
Did you mean to add this symlink?

https://codereview.chromium.org/1204783003/diff/20001/sky/engine/core/paintin...
File sky/engine/core/painting/Canvas.cpp (right):

https://codereview.chromium.org/1204783003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.cpp:221: void Canvas::drawImageRect(const
CanvasImage* image, Rect& src, Rect& dst, Paint* paint) {
I'm not sure this name is immediately clear to me.  Should we just match the
Skia name?

drawBitmapRectToRect?

drawImageRectToRect?

https://codereview.chromium.org/1204783003/diff/20001/sky/engine/core/paintin...
sky/engine/core/painting/Canvas.cpp:226:
m_canvas->drawBitmapRectToRect(image->bitmap(), &src.sk_rect, dst.sk_rect,
&paint->paint());
I think this file uses 4 space indent.

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/g...
File sky/examples/game/lib/game_demo_world.dart (right):

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/g...
sky/examples/game/lib/game_demo_world.dart:380: double baseScaleX =
32.0/_img.size.width;
I thought our style was to have spaces around operators?  I understand you're
not changing that part of this line, but if it is, we should probably fix it.

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/s...
File sky/examples/game/lib/spritesheet.dart (right):

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/s...
sky/examples/game/lib/spritesheet.dart:34: num x = data["x"];
Do you mean to use num instead of int or double?

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/t...
File sky/examples/game/lib/texture.dart (right):

https://codereview.chromium.org/1204783003/diff/20001/sky/examples/game/lib/t...
sky/examples/game/lib/texture.dart:20: frame = new Rect.fromLTRB(0.0, 0.0,
image.width.toDouble(), image.height.toDouble()),
fromPointAndSize ?  I guess Image doesn't have a size accessor?

[eseidel, 2015-06-23 23:28:05]

Assuming you fix the above nits, lgtm.  Please make sure you've addressed ian
and collin's comments. :)

[viktorl, 2015-06-23 23:38:35]

Committed patchset #2 (id:20001) manually as
e9d5f500fbb3e575af25d83d291c403149460f05 (presubmit successful).