CSS Paker: block padding and grouping

[aarondail]

The Overview

Firstly, thanks for making this great tool!

This is kinda a request for either some new functionality or possibly a new extensibility point. Its a bit long, so bear with me!

The Problem

For context, we are using the tool to generate CSS using the packed layout and a svg sprite sheet. It works well. :)

But we have some sprites that represent different states of an icon (i.e. a checkbox-unselected-icon, and a checkbox-selected-icon). We noticed that on some browsers at some non-100% zoom levels, when you interacted with the icon and triggered the other sprite to show (i.e. checked the checkbox) the icon would seem to shift right|left|up|down by a pixel. Its not a huge deal, but visually its kinda jarring.

This is not a SVG specific problem ... it happens with raster sprite sheets too and has been happening for a long time ... because browsers all do different sub-pixel rounding for the sprite's starting screen coordinates when you zoom in/out (note that it does not happen AFAICT if you zoom via touch though)

Anyways, to mitigate this (to some degree) we monkey patched two changes into the lib\svg-sprite\mode\css\packer.js code.

The Request

I'd like to see if you are open to adding this functionality into the tool itself or if we could get an extension point (i.e. pass a function in somewhere) so we could get rid of the monkey patching.

(That said, thank you for designing the code in a way where we at least have the possibility to monkey patch it and get something working!)

The Monkey Patching

The two changes we are patching into packer.js are both in the fit function:

• We group "related" sprites on the same horizontal row (at least jittering happens in only one direction). We took "related" to mean same dimensions and from the same directory. Its naive but sufficient for us.
• We pad all blocks by a variable width/height so they always start (on both axis) on a multiple of X (where X is 5 currently).

These seemed to help mitigate the problem to a large degree. We also customized the css template to use position.absolute.px instead of position.relative.xy and that helped as well.

Here the actual change we made, which may be more useful than my explanation 😄

const alignment = 5;

function pad (c) {
  var paddingNeeded = (alignment - (c % alignment));
  return c + paddingNeeded;
}

SVGSpriteCssPacker.prototype.fit = function() {
  // Blocks basically represent sprites that need to be placed in the sprite sheet.
  var blockGroups = _.chain(this.blocks)
    .groupBy(
      // Group blocks together if they have the same dimensions and come from the same source directory
      (b) => JSON.stringify({ groupName: spriteIdToSourceDirectoryMap[this.shapes[b.index].id], width: b.width, height: b.height })
    ).map((blocksInGroup) => {
      // Then for each group of blocks, calculate the width and height of them combined horizontally
      var group = { width: 0, height: 0, blocks: [] };
      blocksInGroup.forEach((b) => {
        group.width += pad(b.width);
        group.height = Math.max(pad(b.height), group.height);
        group.blocks.push(b);
      });
      return group;
    }).orderBy(
      // Finally sort so larger groups come first
      (b) => Math.max(b.width, b.height),
      ["desc"]
    ).value();

  // This and the following forEach loop are very similar to the function we are monkey patching
  this.root.width = blockGroups.length > 0 ? blockGroups[0].width : 0;
  this.root.height = blockGroups.length > 0 ? blockGroups[0].height : 0;

  blockGroups.forEach((bg) => {
    var node = this._findNode(this.root, bg.width, bg.height);
    var location = node ? this._splitNode(node, bg.width, bg.height) : this._growNode(bg.width, bg.height);
    // This is different than the original function.  Instead of dealing with blocks one at a time we layout these
    // groups, and then iterate over the blocks in the group here.
    bg.blocks.forEach((b) => {
      this.positions[b.index] = {x: location.x, y: location.y};
      location.x += pad(b.width);
    });
  });

  return this.positions;
};

[jkphl]

Hey @aarondail, thanks for your submission! At a glance it sounds reasonable and I will review it further to see how to incorporate it. However, due to capacity reasons I won't be able to work on all this until end of April, so please bear with me. Thanks! :)

[jkphl]

Hey @aarondail, I finally found the time to dive into your issue — thanks a lot for this awesome submission. I really learnt a lot reading your code! :)

I'm currently doing some house cleaning with svg-sprite and will deal with the more fundamental issues first (like updating dependencies), but I'll definitely keep thinking about your proposal. In the meantime, could you please have a look at #142 and tell me if you see any relationship between these issues? Thanks!

[aarondail]

Thanks @jkphl, I took a look at #142, but I think they are independent in the sense that their problem is different than mine...

But as it happens, I also wanted to use absolute pixel coordinates in my sprite sheet so I customized the default template to do that. FWIW, using absolute pixel coordinates seems to also help with the same problem I opened this ticket for (emphasis on "seems to").