phetsims / plinko-probability

"Plinko Probability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 7 forks source link

use 1 shadow for Galton board #67

Closed pixelzoom closed 8 years ago

pixelzoom commented 8 years ago

From Board.js:

    // produces the bottom dropped shadow on the Galton Board
    var boardShadowRectangleGradient = new LinearGradient( ... );
    var horizontalRectangle = new Rectangle( ... );
    this.addChild( horizontalRectangle );

    // create the dropped shadow to the right of the galton board
    var slantedShadowShape = new Shape()....;
    var boardSlantedGradient = new LinearGradient( ...);
    var slantedShadowPath = new Path( slantedShadowShape, ...);
    this.addChild( slantedShadowPath );

This is drawing 2 expensive gradients. And I don't see why the same look can't be accomplished with 1 shadow (with 1 gradient).

In light of other performance issues, this is worth investigating.

veillette commented 8 years ago

FYI, this gradients are used once we instante the board. These are the dropped shadows of the board itself.

pixelzoom commented 8 years ago

A couple of other problematic things in Board:

(1) Option height is passed through to supertype Node, which may have unintended consequences. Since this option is specific to Board, it should have a different name.

(2) All options are assigned to this.options. This pattern is to be generally avoided. And in this case, clients only need the board dimensions, for layout purposes. Board's general width and height properties may suffice for that. If not, then save only the options needed.

pixelzoom commented 8 years ago

Also kind of odd that we have Board (which renders the GaltonBoard background) and GaltonBoardNode (which renders the pegs). Better names might be something like GaltonBoardBackground and PegsNode respectively.

pixelzoom commented 8 years ago

There's also layout built into the shapes for the drop shadow nodes. The layout should be done in options to the nodes, not in the shapes.

pixelzoom commented 8 years ago

Also magic constants in the drop shadows. For example, 10 in this bit:

    var bottomShadowGradient = new LinearGradient( width / 2, height, width / 2, height + 10 )
      .addColorStop( 0.00, options.shadowFill )
      .addColorStop( 0.20, options.shadowFill )
      .addColorStop( 1.00, PlinkoProbabilityConstants.BACKGROUND_COLOR );
    var bottomShadowNode = new Rectangle( faceNode.left + 4, faceNode.bottom, faceNode.width + 8, 10, 5, 50, {
      fill: bottomShadowGradient
    } );

Other magic constants include 4 (horizontal offset of shadows?) , 8 (2x4?), and 5 (?).

pixelzoom commented 8 years ago

Keeping the 2-gradient approach for the shadow. But I did factor layout out of the shapes, and factored out magic numbers. Compared display to previous versions and they match, so no changes visible to user. Closing.