Use defaultValue.EMPTY_OBJECT to avoid unnecessary object creation

[OmarShehata]

In most CesiumJS constructors, you'll see this pattern of creating an options object if one wasn't specified:

https://github.com/AnalyticalGraphicsInc/cesium/blob/f97d63ea010af07d21855e273ac3e0e98397107b/Source/Core/BingMapsGeocoderService.js#L29-L30

We avoid doing options = defaultValue(options, {}); because this will create a new object and throw it away if an options object was passed. This can be a performance/memory issue especially if this constructor is called a lot.

There are a bunch of places where this is not done. A regex search defaultValue\([\w]+, {} shows the following files.

C:\Workspace\cesium\Source\Core\EllipsoidTerrainProvider.js:
   39       */
   40      function EllipsoidTerrainProvider(options) {
   41:         options = defaultValue(options, {});
   42  
   43          this._tilingScheme = options.tilingScheme;

C:\Workspace\cesium\Source\Core\GeographicTilingScheme.js:
   39       */
   40      function GeographicTilingScheme(options) {
   41:         options = defaultValue(options, {});
   42  
   43          this._ellipsoid = defaultValue(options.ellipsoid, Ellipsoid.WGS84);

C:\Workspace\cesium\Source\Core\GoogleEarthEnterpriseTerrainProvider.js:
  117       */
  118      function GoogleEarthEnterpriseTerrainProvider(options) {
  119:         options = defaultValue(options, {});
  120  
  121          //>>includeStart('debug', pragmas.debug);

C:\Workspace\cesium\Source\Core\IonResource.js:
  167      IonResource.prototype.fetchImage = function (options) {
  168          if (!this._isExternal) {
  169:             options = defaultValue(options, {});
  170              options.preferBlob = true;
  171          }

C:\Workspace\cesium\Source\Core\OpenCageGeocoderService.js:
   62          url.setQueryParameters({key: apiKey});
   63          this._url = url;
   64:         this._params = defaultValue(params, {});
   65      }
   66  

C:\Workspace\cesium\Source\Core\WebMercatorTilingScheme.js:
   41       */
   42      function WebMercatorTilingScheme(options) {
   43:         options = defaultValue(options, {});
   44  
   45          this._ellipsoid = defaultValue(options.ellipsoid, Ellipsoid.WGS84);

C:\Workspace\cesium\Source\DataSources\KmlDataSource.js:
 2488       */
 2489      function KmlDataSource(options) {
 2490:         options = defaultValue(options, {});
 2491          var camera = options.camera;
 2492          var canvas = options.canvas;
 ....
 2699          //>>includeEnd('debug');
 2700  
 2701:         options = defaultValue(options, {});
 2702          DataSource.setLoading(this, true);
 2703  

C:\Workspace\cesium\Source\Renderer\Context.js:
  192  
  193          options = clone(options, true);
  194:         options = defaultValue(options, {});
  195          options.allowTextureFilterAnisotropic = defaultValue(options.allowTextureFilterAnisotropic, true);
  196          var webglOptions = defaultValue(options.webgl, {});

C:\Workspace\cesium\Source\Renderer\RenderState.js:
   90       */
   91      function RenderState(renderState) {
   92:         var rs = defaultValue(renderState, {});
   93          var cull = defaultValue(rs.cull, {});
   94          var polygonOffset = defaultValue(rs.polygonOffset, {});

C:\Workspace\cesium\Source\Scene\ArcGisMapServerImageryProvider.js:
  109       */
  110      function ArcGisMapServerImageryProvider(options) {
  111:         options = defaultValue(options, {});
  112  
  113          //>>includeStart('debug', pragmas.debug);

C:\Workspace\cesium\Source\Scene\BingMapsImageryProvider.js:
   93       */
   94      function BingMapsImageryProvider(options) {
   95:         options = defaultValue(options, {});
   96  
   97          //>>includeStart('debug', pragmas.debug);

C:\Workspace\cesium\Source\Scene\Cesium3DTileBatchTable.js:
   91              extensions = batchTableJson.extensions;
   92          }
   93:         this._extensions = defaultValue(extensions, {});
   94  
   95          var properties = initializeProperties(batchTableJson);

C:\Workspace\cesium\Source\Scene\createOpenStreetMapImageryProvider.js:
   60       */
   61      function createOpenStreetMapImageryProvider(options) {
   62:         options = defaultValue(options, {});
   63  
   64          var url = defaultValue(options.url, 'https://a.tile.openstreetmap.org/');

C:\Workspace\cesium\Source\Scene\createTileMapServiceImageryProvider.js:
   84       */
   85      function createTileMapServiceImageryProvider(options) {
   86:         options = defaultValue(options, {});
   87  
   88          //>>includeStart('debug', pragmas.debug);

C:\Workspace\cesium\Source\Scene\GoogleEarthEnterpriseMapsProvider.js:
  104       */
  105      function GoogleEarthEnterpriseMapsProvider(options) {
  106:         options = defaultValue(options, {});
  107  
  108          //>>includeStart('debug', pragmas.debug);

C:\Workspace\cesium\Source\Scene\ImageryLayer.js:
  163          this._imageryProvider = imageryProvider;
  164  
  165:         options = defaultValue(options, {});
  166  
  167          /**

C:\Workspace\cesium\Source\Scene\PointCloudShading.js:
   24       */
   25      function PointCloudShading(options) {
   26:         var pointCloudShading = defaultValue(options, {});
   27  
   28          /**

C:\Workspace\cesium\Source\Scene\processModelMaterialsCommon.js:
   25       */
   26      function processModelMaterialsCommon(gltf, options) {
   27:         options = defaultValue(options, {});
   28  
   29          if (!defined(gltf)) {

C:\Workspace\cesium\Source\Scene\processPbrMaterials.js:
   25       */
   26      function processPbrMaterials(gltf, options) {
   27:         options = defaultValue(options, {});
   28  
   29          // No need to create new techniques if they already exist,

C:\Workspace\cesium\Source\Scene\SingleTileImageryProvider.js:
   50       */
   51      function SingleTileImageryProvider(options) {
   52:         options = defaultValue(options, {});
   53          //>>includeStart('debug', pragmas.debug);
   54          if (!defined(options.url)) {

C:\Workspace\cesium\Source\Widgets\CesiumWidget\CesiumWidget.js:
  204          container = getElement(container);
  205  
  206:         options = defaultValue(options, {});
  207  
  208          //Configure the widget DOM elements

This would just be a matter of replacing all those with defaultValue(options, defaultValue.EMPTY_OBJECT). @mramato can you label with "Good first issue" ?

[abu271]

Hi @OmarShehata,

I can happily solve this issue for you :)

Kind regards,

Abu

[OmarShehata]

@abuDarda97 that would be awesome! Check out the build guide, let me know if you have any questions.

[abu271]

Hi @OmarShehata in the RenderState.js file under the Renderer folder should I replace all the empty objects with defaultValue.EMPTY_OBJECT.

    var rs = defaultValue(renderState, {});
    var cull = defaultValue(rs.cull, {});
    var polygonOffset = defaultValue(rs.polygonOffset, {});
    var scissorTest = defaultValue(rs.scissorTest, {});
    var scissorTestRectangle = defaultValue(scissorTest.rectangle, {});
    var depthRange = defaultValue(rs.depthRange, {});
    var depthTest = defaultValue(rs.depthTest, {});
    var colorMask = defaultValue(rs.colorMask, {});
    var blending = defaultValue(rs.blending, {});
    var blendingColor = defaultValue(blending.color, {});
    var stencilTest = defaultValue(rs.stencilTest, {});
    var stencilTestFrontOperation = defaultValue(stencilTest.frontOperation, {});
    var stencilTestBackOperation = defaultValue(stencilTest.backOperation, {});
    var sampleCoverage = defaultValue(rs.sampleCoverage, {});

Also, which testing scripts should I run?

[OmarShehata]

@abuDarda97 that sounds right. For the tests, see the testing guide here https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Documentation/Contributors/TestingGuide/README.md#running-the-tests

[abu271]

Hey @OmarShehata I carried out all of the changes, ran all of the tests got 0 failures. However, I have 4 specs on pending.

Is it okay to make a PR?

[OmarShehata]

@abuDarda97 yes, that's normal! The "pending" specs are ones that are actually just intentionally disabled. I wonder if it'd be worth adding a note on that in the testing guide since that was something I was confused about as well when I first ran the tests.

But yes, go ahead and open the PR!

[OmarShehata]

This was done in #7793.