phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

Improve MaterialView #305

Closed samreid closed 1 month ago

samreid commented 1 month ago

During https://github.com/phetsims/density-buoyancy-common/issues/123 I observed:

class AluminumMaterialView extends MaterialView {
  public constructor() {
    super( new THREE.MeshStandardMaterial( {
      map: aluminumColorTexture,
      normalMap: aluminumNormalTexture,
      normalScale: new THREE.Vector2( 1, -1 ),
      roughnessMap: aluminumRoughnessTexture,
      metalnessMap: aluminumMetalnessTexture,
      envMap: getEnvironmentTexture()
    } ) );
  }
}

For this code, is it important to create a new instance for each Material?

export class DensityMaterials {
  /**
   * Returns a view for the given Material.
   */
  public static getMaterialView( material: Material ): MaterialView {
    if ( material === Material.ALUMINUM ) {
      return new AluminumMaterialView();
    }
samreid commented 1 month ago

MaterialView disposes a material when the MaterialView is disposed. That seems odd. Are they indeed disposed?

Oh, it is a THREE.Material, not a Material.

samreid commented 1 month ago

The ternary operator helped clean things up a little bit. I tried inlining the material views, but that was worse. Classes are good enough, no good reason to switch to methods. Closing.