Carousel should support objects of different sizes

[samreid]

Carousel is currently optimized for items of the same size. When objects are different size, the layout strategy will lead to unequal spacing as documented in https://github.com/phetsims/circuit-construction-kit-dc/issues/91. See https://github.com/phetsims/circuit-construction-kit-dc/issues/91#issuecomment-317186449 in particular for pics of this phenomenon.

[samreid]

In https://github.com/phetsims/circuit-construction-kit-dc/issues/91#issuecomment-317205783 @pixelzoom said:

You're calling this "unequal spacing", but the items are actually equally spaced. Spacing is measured from center to center of each item. And the spacing must be equal in order to ensure that each page of the carousel is the same length, so that pages can be scrolled. If you try to do what @ariel-phet has suggested, you're going to end up with either a very complicated change to Carousel, or some really ugly client code. My recommendation is to (1) mitigate the problem by increasing options.spacing a bit so the "Light Bulb" item doesn't look as cramped, and (2) close this item as "won't fix".

[samreid]

I originally investigated a solution like this:

    spacingAdjustments: {}, // {Object} map from Node.id {number} => spacing adjustment {number}.  Allow the spacing of
                            // each item to be adjusted without disrupting the overall layout

...

      // Allow the spacing of each object to be adjusted without disrupting the overall layout
      var spacingAdjustment = options.spacingAdjustments[ item.id ] || 0;

      // add the item
      if ( isHorizontal ) {
        item.centerX = itemCenter + spacingAdjustment;
        item.centerY = options.margin + ( maxItemHeight / 2 );
      }
      else {
        item.centerX = options.margin + ( maxItemWidth / 2 );
        item.centerY = itemCenter + spacingAdjustment;
      }

The client code looked like this:

    var spacingAdjustments = {};
    spacingAdjustments[ rightBatteryToolNode.id ] = -7;
    spacingAdjustments[ resistorToolNode.id ] = +7;
    if ( highVoltageBatteryToolNode ) {
      spacingAdjustments[ highVoltageBatteryToolNode.id ] = -7;
      spacingAdjustments[ highResistanceResistorToolNode.id ] = -10;
      spacingAdjustments[ highResistanceBulbToolNode.id ] = -7;
    }
    spacingAdjustments[ dollarBillNode.id ] = -6;
    spacingAdjustments[ paperClipNode.id ] = -7;
    spacingAdjustments[ eraserToolNode.id ] = +3;

    // If there are 2+ pages, show them in a carousel
    this.carousel = new Carousel( circuitElementToolNodes, {
      spacingAdjustments: spacingAdjustments,

And the view looked like this:

[samreid]

The above approach seems like it would allow complete flexibility in the positioning of objects, but it requires a lot of manual adjustment on the simulation side. I wonder what a solution would look like that maintains a constant spacing between object edges rather than between their centers.

[ariel-phet]

@samreid just to be clear, this is certainly a lower priority issue to me. Lots of fish to fry on CCK, and this is a visual tweak that will become important to me once we are close to publication and other bugs are all worked out. Recommend not spending time on it for the moment.

[samreid]

@samreid just to be clear, this is certainly a lower priority issue to me.

Sounds fine, I'll just jot down some thoughts before turning my attention elsewhere. I investigated the idea of setting the spacing between top/bottom (can be generalized to left/right) and it looked something like this:

    items.forEach( function( item ) {

      // add the item
      if ( isHorizontal ) {
        item.centerX = nextPosition;
        item.centerY = options.margin + ( maxItemHeight / 2 );
      }
      else {
        item.centerX = options.margin + ( maxItemWidth / 2 );
        item.top = nextPosition;
      }
      scrollingNode.addChild( item );

      // center for the next item
      nextPosition = item.bottom + options.spacing;

And the spacing came out like this:

This could take longer to implement, like 4-6 hours depending on how many Carousel clients may need to be updated. This approach may work well for both equally sized and unequally sized objects.

[samreid]

Reassigned to @ariel-phet to evaluate the approaches identified above, and to prioritize, delegate and schedule.

[samreid]

The time estimate for the approach in https://github.com/phetsims/sun/issues/307#issuecomment-317562469 is about 15 minutes (plus code review cycle).

[arouinfar]

@samreid I think the "after" in https://github.com/phetsims/sun/issues/307#issuecomment-317562469 looks really nice.

[ariel-phet]

@pixelzoom mind chiming in on the proposed approach. I think being able to set spacing top and bottom might be quite nice when dealing with carousels that have these objects with various sizes (which will happen more).

[samreid]

I'd also love to hear from @pixelzoom before further investigation on the strategy to deal with spacing between object edges rather their centers--it seems liable to have a page end halfway across an item (or lead to gaps). We may need to live with an adjustment pattern?

[ariel-phet]

@samreid could things be set up, so spacing between centers is the default (works well for all objects the same size) and an option for using spacing between edges? (Or is that what you were already proposing)?

[samreid]

@samreid could things be set up, so spacing between centers is the default (works well for all objects the same size) and an option for using spacing between edges? (Or is that what you were already proposing)?

It would be simplest if one strategy worked well for for all cases, but not too crazy if we need to support 2 strategies.

For the new strategy, what if you specify the number of items per page but not the spacing between items (well, perhaps a minimum spacing), and it will automatically position them have the correct edge-edge (or center-center) spacing?

[samreid]

In discussion with @jonathanolson, he recommended the best long term solution for Carousel would be to use HBox/VBox style spacing (between edges of objects). This would need to detect the proper padding to align things, you would need to pass in a minimum spacing, and it may need to add a little extra spacing on some pages so the alignment would look correct. If the last page is unfilled, it would need a custom spacing. It would also be good for new UI components to support resizing. And things should be aligned when pages are full.

For now, @jonathanolson demonstrated how we can use VBox to create custom pages for a carousel (with one item per page). I'll clean it up and commit shortly (for CCK only).

It will still be best to confer with @pixelzoom before taking action.

[pixelzoom]

You (collectively) seem to feel that this is worth complicating Carousel. I don't share that opinion. When you decide on an implementation, have tested it thoroughly, and have added a demonstration to sun's demo application, then I'll be happy to review the changes.

[samreid]

I'm satisfied with the workaround applied in Circuit Construction Kit, so this is not a high priority issue for me. Generally, it seems like Carousel should gracefully accommodate objects of different size. I'll leave it up to @ariel-phet to decide if/when to proceed with that.

[ariel-phet]

@samreid sounds good. Will unassign for the moment, and we can revisit as needed (will likely come up again in further CCK incarnations).

[jonathanolson]

I removed the REVIEW note in CCK.

I'd like to state that, for the record, I believe this may be valuable behavior, even if it becomes a separate "Carousel" implementation that doesn't complicated the original.

Waiting for the next usage (in another sim) sounds workable to me.

[samreid]

From https://github.com/phetsims/circuit-construction-kit-common/issues/753#issuecomment-953252956

@ariel-phet, @arouinfar and I discussed this today and it seems like for differently sized items, the carousel should keep a consistent distance between the bottom of the previous item and the top of the next item.

This will be a larger amount of work because many sims are using Carousel, and we wouldn't want to disrupt them. Therefore we won't do it for AC 1.0, but we would like to investigate the spacing rule described above in a future version. I'll open a common code issue for discussion.