8261 - Avoid including Cesium3DTileSet in Picking.js #8532

Samulus · 2020-01-13T16:46:22Z

cesium-concierge · 2020-01-13T16:46:27Z

Thank you so much for the pull request @Samulus! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

hpinkos · 2020-01-13T16:49:30Z

Can we do something similar to avoid sucking in Cesium3DTileFeature too?

lilleyse · 2020-01-13T17:05:51Z

@hpinkos Cesium3DTileFeature doesn't have too many imports and will unlikely have more. It might be ok to skip it.

import Color from '../Core/Color.js';
import defined from '../Core/defined.js';
import defineProperties from '../Core/defineProperties.js';

lilleyse

Just a few minor style things.

I'm going to verify that this actually reduces the build size and then it should be good to go.

lilleyse · 2020-01-13T18:10:03Z

Source/Scene/Cesium3DTileset.js

@@ -879,6 +879,19 @@ import TileOrientedBoundingBox from './TileOrientedBoundingBox.js';
    }

    defineProperties(Cesium3DTileset.prototype, {
+


Style: remove empty line

Source/Scene/Cesium3DTileset.js

lilleyse · 2020-01-13T18:10:27Z

Source/Scene/Cesium3DTileset.js

+
+        /**
+         * NOTE: This getter exists so that `Picking.js` can differentiate between
+         *       PrimitiveCollections and Cesium3DTileset objects without inflating


Suggested change

* PrimitiveCollections and Cesium3DTileset objects without inflating

* PrimitiveCollection and Cesium3DTileset objects without inflating

lilleyse · 2020-01-13T18:10:37Z

Source/Scene/Cesium3DTileset.js

+        /**
+         * NOTE: This getter exists so that `Picking.js` can differentiate between
+         *       PrimitiveCollections and Cesium3DTileset objects without inflating
+         *       the size of the module via `instance of Cesium3DTileset`


Suggested change

* the size of the module via `instance of Cesium3DTileset`

* the size of the module via `instanceof Cesium3DTileset`

lilleyse · 2020-01-13T18:11:03Z

Source/Scene/Cesium3DTileset.js

+         *       the size of the module via `instance of Cesium3DTileset`
+         */
+
+        isCesium3DTileset: {


Style:

Suggested change

isCesium3DTileset: {

isCesium3DTileset : {

lilleyse · 2020-01-13T18:11:15Z

Source/Scene/Cesium3DTileset.js

+         */
+
+        isCesium3DTileset: {
+            get: function() {


Style:

Suggested change

get: function() {

get : function() {

lilleyse · 2020-01-13T18:12:45Z

Source/Scene/Cesium3DTileset.js

+            get: function() {
+                return true;
+            }
+        }


This is why CI is failing. You can run npm run eslint locally which will also catch this.

Suggested change

}

},

lilleyse · 2020-01-13T18:16:40Z

Also - add an entry to CHANGES.md.

Samulus · 2020-01-13T18:27:21Z

@lilleyse fixed

lilleyse · 2020-01-13T18:39:55Z

Can confirm this fix reduces the build size. For future reference these were the steps @mramato suggested to verify this:

Switch to master
Edit CesiumViewer.js to create a minimal demo:

    window.CESIUM_BASE_URL = '../../Source/';

    import {
        CesiumWidget
    } from '../../Source/Cesium.js';

    function main() {
        var widget = new CesiumWidget('cesiumContainer');
    }

    main();

Run npm run makeZipFile
Run npm run buildApps
Check size of Build/Apps/CesiumViewer/CesiumViewer.js
Do the steps above on this branch

With fix: CesiumViewer.js is 1.3 MB
Without fix: CesiumViewer.js is 1.8 MB

lilleyse · 2020-01-13T18:41:36Z

@Samulus there's one style comment that you missed: https://github.com/AnalyticalGraphicsInc/cesium/pull/8532/files#r365949948

After that's fixed I'll go ahead and merge.

lilleyse · 2020-01-13T21:38:08Z

Source/Scene/Cesium3DTileset.js

+         *       PrimitiveCollection and Cesium3DTileset objects without inflating
+         *       the size of the module via `instanceof Cesium3DTileset`
+         */
+        isCesium3DTileset : {


My mistake for not noticing this in the initial review - the getter should be marked as @private.

This reverts commit cc88320.

Samulus · 2020-01-13T21:48:29Z

@lilleyse rebased and annotation added

lilleyse · 2020-01-13T22:12:05Z

Little bit of offline miscommunication on my part, I should have been the one to merge the PR. That said, the changes do look good and I was about to merge 👍

Samulus force-pushed the 8261/instance_of_picking branch from 497e429 to e269701 Compare January 13, 2020 16:47

lilleyse reviewed Jan 13, 2020

View reviewed changes

Samuel Vargas added 8 commits January 13, 2020 16:46

8261 - Avoid including Cesium3DTileSet in Picking.js

f26c6fe

8261 - Avoid including Cesium3DTileFeature, fix imports

f13c7d3

Revert "8261 - Avoid including Cesium3DTileFeature, fix imports"

cea858b

This reverts commit cc88320.

8261 - Remove unused Cesium3DTileset import

0f9c98b

8261 - Convert boolean property to getter

d1cad88

8261 - Fix style issues, update CHANGES.md

a3f6962

8261 - Remove empty line between comment and property

6eb98b0

8261 - Add @Private annotation

96797bf

Samulus force-pushed the 8261/instance_of_picking branch from 86f8c58 to 96797bf Compare January 13, 2020 21:47

Merge branch 'master' into 8261/instance_of_picking

4cb0cb9

Samulus merged commit dffefca into master Jan 13, 2020

Samulus deleted the 8261/instance_of_picking branch January 13, 2020 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8261 - Avoid including Cesium3DTileSet in Picking.js #8532

8261 - Avoid including Cesium3DTileSet in Picking.js #8532

Samulus commented Jan 13, 2020

cesium-concierge commented Jan 13, 2020

hpinkos commented Jan 13, 2020

lilleyse commented Jan 13, 2020

lilleyse left a comment

lilleyse Jan 13, 2020

lilleyse Jan 13, 2020

lilleyse Jan 13, 2020

lilleyse Jan 13, 2020

lilleyse Jan 13, 2020

lilleyse Jan 13, 2020

lilleyse commented Jan 13, 2020

Samulus commented Jan 13, 2020

lilleyse commented Jan 13, 2020 •

edited

Loading

lilleyse commented Jan 13, 2020

lilleyse Jan 13, 2020

Samulus commented Jan 13, 2020

lilleyse commented Jan 13, 2020

		@@ -879,6 +879,19 @@ import TileOrientedBoundingBox from './TileOrientedBoundingBox.js';
		}

		defineProperties(Cesium3DTileset.prototype, {

	* PrimitiveCollections and Cesium3DTileset objects without inflating
	* PrimitiveCollection and Cesium3DTileset objects without inflating

	* the size of the module via `instance of Cesium3DTileset`
	* the size of the module via `instanceof Cesium3DTileset`

8261 - Avoid including Cesium3DTileSet in Picking.js #8532

8261 - Avoid including Cesium3DTileSet in Picking.js #8532

Conversation

Samulus commented Jan 13, 2020

cesium-concierge commented Jan 13, 2020

hpinkos commented Jan 13, 2020

lilleyse commented Jan 13, 2020

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

lilleyse commented Jan 13, 2020

Samulus commented Jan 13, 2020

lilleyse commented Jan 13, 2020 • edited Loading

lilleyse commented Jan 13, 2020

lilleyse Jan 13, 2020

Choose a reason for hiding this comment

Samulus commented Jan 13, 2020

lilleyse commented Jan 13, 2020

lilleyse commented Jan 13, 2020 •

edited

Loading