button content offset should be handled in threeDAppearanceStrategy

[pixelzoom]

This issue was factored out of #234 and #235.

Instead of setting xContentOffset and yContentOffset in (for example) ResetButton to compensate for the gradient in threeDAppearanceStrategy, that compensation should be handled (with some default values) in threeDAppearanceStrategy. It's not immediately clear to me how to do this.

@jbphet Since I took care of making ResetButton look good for flatAppearanceStrategy, how about if you handle this issue?

[pixelzoom]

Other buttons that are compensating for 3D by setting xContentOffset and yContentOffset include:

scenery-phet.LeftRightSpinner
scenery-phet.StepButton
scenery-phet.StepBackButton
energy-skate-park-basics.ControlPointUI.deleteButton

[jbphet]

In #235, @pixelzoom compensated for the look of the reset button by adjusting the local bounds of the icon. He essentially reflected the original shape, merged the two shapes, and then used the combined bounds, which then looks reasonably centered on the flat strategy. The following image illustrates:

Modifying the local bounds works, but doesn't strike me as a very general solution, and may also be a bit tricky for future developers to figure out. Here is an alternative proposal: we modify the code so that the xContentOffset and yContentOffset are handled in the appearance strategies, are documented to indicate that they are intended for the flat strategy, then add additional compensation in the 3D appearance strategy. We may be able to make the 3D compensation general enough that it doesn't need to appear in the options but, if we can't, we would have some additional options like threeDCompensationX and threeDCompensationY. Only the 3D strategy would pay attention to these values.

Also, I agree with a comment that @pixelzoom made elsewhere that it would be good to rename xContentOffset and it's Y counterpart to something like contentOffsetX or contentXOffset.

Back to @pixelzoom for input before I make any moves on this.

[pixelzoom]

Modifying the local bounds works, but doesn't strike me as a very general solution,

@jbphet I think you're confusing 2 unrelated issues. Creating a centerable icon and adjusting the offset for 3D appearance are two separate issues. You start with a centerable icon (as we now have for ResetButton), then compensate for centering in 3D appearance.

In the case of how the ResetButton icon is now created, it's not supposed to be a general solution. It's the centering solution that's specific to the icon for this button. And it's the solution that was recommended by @jonathanolson and @samreid as being most efficient. Depending on the button, each button may have to employ some technique to make the icon centerable - but there will be no general solution to this.

[pixelzoom]

Summary of Skype voice discussion with @jbphet:

In both flat and 3D appearance strategy, the content is centered on the button, subject to some offsets. xContentOffset and yContentOffset (the content offset) are applicable to both flat and 3D appearance strategies, and their default values are zero.

In the case of flat appearance strategy, the content offsets are applied without modification. In the case of 3D appearance strategy, an additional "offset correction" will be applied that is a function of the button gradient. Hopefully this 3D offset correction will prove to be applicable to all cases, and can remain internal. If not, then @jbphet recommends adding additional options to adjust the 3D correction offset.

In the general case, a button designer should ensure that their button looks good with flat appearance strategy, by either (a) providing a content Node that can be centered without adjusting xContentOffset and yContentOffset, or (b) providing values for xContentOffset and yContentOffset that are determined empirically. Both options should be available, because they are appropriate for different situations.

Option (a) is appropriate when the content Node can be created programmatically. And the ResetButton is such a case. It's trivial to programmatically adjust localBounds such that the icon's center is the center of the circle. To do this using xContentOffset and yContentOffset would involve (and have involved) magic constants that are specific to the options provided to ResetShape. If you tweak ResetShape, you must tweak xContentOffset and yContentOffset empirically. So in this case, setting localBounds is a general and robust solution to the problem.

Options (b) is appropriate when it's not possible to programmatically create a center-able icon. StepButton may be such a button. In these cases, the best solution is to set xContentOffset and yContentOffset values that are determined empirically.

[pixelzoom]

I'm smacking up against this issue again in phetsims/scenery-phet#243, where the only reason for having separate "forward" and "back" step buttons is because their xContent option needs to be different to work around this issue.

[pixelzoom]

This is an issue once again in plinko-probability, see phetsims/plinko-probability#26 (comment). If we're going to use 'flat' as a means of addressing performance issues, then we need to allocate some time to address this issue.

[pixelzoom]

Discussed via Skype with @jbphet. He asked me to label this for developer meeting, specifically to discuss prioritization.

[pixelzoom]

9/1/16 dev meeting:
@ariel-phet does want to address this, medium-low priority. Revisit after @jbphet makes some progress on expression-exchange.

[pixelzoom]

Not going to do this. As I'm discovering in #643 and #649, this would be way too much work. And what really should be done is "sun buttons 2.0", a rewrite of sun buttons.

[samreid]

There was a stray TODO in the code, but based on the prior comment, I decided to convert it to a NOTE.