New layout for sprites, heavily "packed" #1433

Open
wants to merge 67 commits into
from

Conversation

Projects
None yet
5 participants

much more compact than "smart" when images' height and width are differently sized.

All credit goes to Jakes Gordon for the algorithm (and most of the code).

PackFitter algorithm description:
http://codeincomplete.com/posts/2011/5/7/bin_packing/

code adapted from:
https://github.com/jakesgordon/sprite-factory/blob/master/lib/sprite_factory/layout/packed.rb

See Issue #1422

Comparison images

layout: smart
mysprite-s792cf60370

layout: packed
mysprite-sf32dae54b5

New layout for sprites, heavily "packed"
much more compact than "smart" when images' height and width are differently sized.

All credit goes to Jakes Gordon for the algorithm (and most of the code).

PackFitter algorithm description:
http://codeincomplete.com/posts/2011/5/7/bin_packing/

code adapted from:
https://github.com/jakesgordon/sprite-factory/blob/master/lib/sprite_factory/layout/packed.rb

Haven't done benchmarks on it yet, but for the sample rectangles I gave, the generation times are pretty much identical. with the packed being 20 bytes smaller (282 vs 262).

I expect real world cases will yield much better compression since it really does pack quite a bit better in exchange for more memory usage (could be definitely be optimized further, by dropping the generated hash tree after packing completes).

Owner

scottdavis commented Nov 1, 2013

Looks good I'm ok with just replacing smart packing all together ping @chriseppstein

Did you consider spacing for sprite's elements?

Spacing/margins wasn't built in because I didn't see any functionality for that in smart packing. It just pulls Image#height and Image#width to do the calculations.

It's simple enough to add though, just need to add the margin pixels to the height and width; the core calculations will still remain the same.

er1z commented Nov 15, 2013

@kiafaldorius - the image weight isn't everything. The less pixels on the image are, the less memory they take (I mean browser right now).

Any picture - nevertheless compression - is decoded to the bitmap in memory. Small sprites - no problem. But the larger ones...

Owner

chriseppstein commented Dec 2, 2013

This looks great.

How does this algorithm's performance compare to the current smart packing algorithm with dozens of images in the sprite?

Assuming comparable performance, I'm ok with simply bumping the sprite version and making this the smart algorithm.

The fact that spacing wasn't supported in the smart algorithm was a bug. I'd like to not carry that bug into this algorithm.

Owner

scottdavis commented Dec 2, 2013

Actually the spacing thing wasn't a bug it was just not implemented. there were feature requests but no one ever picked up the ball and finished it. The docs made it clear that configurations for smart pack spacing, padding etc were not supported :)

Sent from my iPhone

On Dec 2, 2013, at 6:36 PM, Chris Eppstein notifications@github.com wrote:

This looks great.

How does this algorithm's performance compare to the current smart packing algorithm with dozens of images in the sprite?

Assuming comparable performance, I'm ok with simply bumping the sprite version and making this the smart algorithm.

The fact that spacing wasn't supported in the smart algorithm was a bug. I'd like to not carry that bug into this algorithm.


Reply to this email directly or view it on GitHub.

@ghost ghost assigned scottdavis Dec 3, 2013

When I find some time--possibly this weekend--I'll hook up spacing and throw out a few benchmarks with random icons from the openicon library. I'm confident the height and width will be smaller, whether the actual filesize is substantially smaller we'll have to see.

Since this thing benefits from proper sizing, I'm considering allowing per-side spacing, eg: spacing-top, spacing-bottom, spacing-left, spacing-right.

Smart Sprite:
46055
real 0m12.532s
user 0m3.170s
sys 0m3.907s
mysprite-s5743b235ff

Packed Sprite:
45937
real 0m11.299s
user 0m2.780s
sys 0m4.467s
mysprite-sd573c13ba3

Smart Sprite:
128590
real 0m12.422s
user 0m4.857s
sys 0m3.460s
mysprite-sbb0f6f07d7

Packed Sprite:
129826
real 0m12.349s
user 0m4.463s
sys 0m3.727s
mysprite-s68aa5a03ee

Smart Sprite:
240453
real 0m14.989s
user 0m5.997s
sys 0m4.297s
mysprite-s53debc97e6

Packed Sprite:
244155
real 0m14.353s
user 0m5.920s
sys 0m3.980s
mysprite-s96c779cbab

So it appears extended square packing might not play so well with png's compression. Which I somewhat expected...But packed might be slightly faster, which is not what I would have thought.

So, with that in mind, it may be wise to keep them separate.

Pack's pixel counts are always less than the smart, so presumably it would still be much more memory efficient to render...at the cost of around 1-2% more filesize (filesize -> bandwidth, so slightly more transit time) for large file counts compared to smart. For two dozen or so, packed is still usually smaller than smart.

I've just pushed my current changes. Commits 68a1f2b and 3ffd7b9 add spacing to the existing smart layout without touching the packed version, so should be safe to cherry pick if you decide not to take the new algorithm. (I had lots of test failures/errors show up when trying to bump the sprite version, probably to do with hardcoded checksums. I'll leave that part to someone in the dev team.)

Owner

chriseppstein commented Aug 18, 2014

I like this, but we need to get it rebased on top of 1.0.

Owner

chriseppstein commented Aug 18, 2014

Actually, on top of #1771 after it lands.

Hey, sorry for late reply, been busy lately.

What's the status of #1771 ? Can I start rebasing it or will I need to wait until it gets into production? (It appears that pull is not passing travis?)

Owner

scottdavis commented Sep 23, 2014

@kiafaldorius you can rebase aginst that the tests are erroring atm due to some travis weirdness but they pass localy just fine

scottdavis and others added some commits Sep 24, 2014

Merge remote-tracking branch 'kia/packing-sprites' into decompose_spr…
…ites

Conflicts:
	lib/compass/sass_extensions/sprites.rb
	sprites/lib/compass/sprites/sass_extensions/image.rb
	sprites/test/units/image_row_test.rb
	sprites/test/units/layout_test.rb
	sprites/test/units/row_fitter_test.rb

* had to update Gemfile because ffi-1.9.4 was yanked from rubygems

Merged in the changes from decomposed_sprites branch in #1771 The sprite tests pass. I had to update the Gemfile's ffi though, 1.9.4 got yanked from rubygems, replaced with 1.9.5 @scottdavis

Owner

scottdavis commented Sep 29, 2014

Awesome

Sent from my iPhone

On Sep 29, 2014, at 5:47 PM, Kia Kroas notifications@github.com wrote:

Merged in the changes from decomposed_sprites branch in #1771 The sprite tests pass. I had to update the Gemfile's ffi though, 1.9.4 got yanked from rubygems, replaced with 1.9.5 @scottdavis


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment