Export public API via index.js #157

lasalvavida · 2016-08-10T17:44:22Z

~~All lib/*/.js functions are exposed via index.js. Also adds a gulp task for regenerating this file.~~

A collection of functions are exposed as the public API. They are all documented.

lasalvavida · 2016-08-10T17:45:03Z

Resolves #153

pjcozzi · 2016-08-10T19:16:55Z

index.js

-    writeBinaryGltf : require('./lib/writeBinaryGltf'),
-    writeGltf : require('./lib/writeGltf')
-};
+    AccessorReader : require('./lib/AccessorReader.js'),


Are you sure you want to expose all of this? That is a lot of API surface area. We learned a very hard lesson in Cesium to only expose what is needed to keep the maintain cost way down.

Please only expose what is currently needed and make sure the public API has full reference doc that can be generated like we do for Cesium. Don't worry about the generated doc's styling or hosting right now.

There's still way too much API surface area here. @lilleyse and @lasalvavida can you please take a pass over this?

I can help if needed, but I would rather you guys get the experience working with APIs. At this point, I would only expose top-level stages and helpers that @likangning93 needs. Anything else is a high-cost loan for money we don't need.

lasalvavida · 2016-08-11T17:36:27Z

I've gone through and trimmed down what we are putting into the public API. Everything exposed now has documentation. It probably isn't perfect, but it is a good starting point.

Things that are exposed: "stages", and helper functions that are fairly glTF specific i.e. byteLengthForComponentType, but not triangleAxisAlignedBoundingBoxOverlap since it is more geometry focused.

lasalvavida · 2016-08-11T17:37:43Z

I also moved addLightDefaults to modelMaterialsCommon to resolve #158. There was a TODO for this.

lasalvavida · 2016-08-11T17:40:27Z

@likangning93 I have exposed everything listed in #153, let me know if there's anything else you want exposed that I missed.

lasalvavida · 2016-08-11T17:42:39Z

Also making a note here that combinePrimitives is not currently exposed. It should be exposed once #108 is resolved.

lilleyse · 2016-08-12T16:12:02Z

index.js

+    writeBinaryGltf : require('./lib/writeBinaryGltf.js'),
+    writeBufferComponentType : require('./lib/writeBufferComponentType.js'),
+    writeGltf : require('./lib/writeGltf.js')
+};


The main problem with exposing a lot of these functions is that they may rely on previous steps, like addPipelineExtras. Either we need to handle that in the functions themselves, or expect users to call things like addPipelineExtras, loadGltfUris, etc on their own.

Would it be enough to just put the dependencies in the doc until we work out custom pipelines?

Yeah I think that's ok for now.

lilleyse · 2016-08-12T17:26:10Z

I just made minor edits. Thanks for adding all this documentation and fixing a bunch of problems you noticed.

I organized some of the requires to follow the node guide, where Cesium functions are set after all the requires:

var Cesium = require('cesium');
var restifyErrors = require('restify-errors');
var aFunction = require('./lib/aFunction');
var bFunction = require('./lib/bFunction');

var Cartesian3 = Cesium.Cartesian3;
var Ellipsoid = Cesium.Ellipsoid;
var ResourceNotFoundError = restifyErrors.ResourceNotFoundError;

lilleyse · 2016-08-12T17:29:15Z

lib/gltfPipeline.js

+ * Add pipeline extras and load uris, then process the gltf.
+ *
+ * @param {Object} gltf A javascript object holding a glTF hierarchy.
+ * @param {Object} options Options to apply to stages during optimization.


In some areas like this the options aren't documented.

We pass options to stages and they should be documented for all of those. I think it would be a bit verbose to document all of the different stage options here.

Well it may be redundant in some cases. Maybe just make a note to refer to the processJSONWithExtras doc.

Ah you beat me to it.

lasalvavida · 2016-08-12T18:25:00Z

@lilleyse updated

lilleyse · 2016-08-12T19:01:47Z

The new changes look good. @pjcozzi feel free to do a final check.

lilleyse · 2016-08-12T19:14:35Z

index.js

+    getUniqueId : require('./lib/getUniqueId.js'),
+    gltfPipeline : require('./lib/gltfPipeline.js'),
+    loadGltfUris : require('./lib/loadGltfUris.js'),
+    mergeBuffers : require('./lib/mergeBuffers.js'),


I just noticed that there isn't doc for this one.

Never mind...

pjcozzi · 2016-08-12T21:06:46Z

Add instructions for building doc to the README.md.

OptimizationStatistics

pjcozzi · 2016-08-17T12:10:38Z

Throughout, the doc needs a lot of work. I am fine with merging this to get the mechanics and then writing more solid doc for a code sprint or something like that, see #163.

@lasalvavida before we merge this, can you please take a pass through our Documentation Guide to see if there are other things we should get into this PR?

pjcozzi · 2016-08-17T12:29:51Z

lib/writeBufferComponentType.js

+ * @param {Number} componentType WebGLConstants value for component type.
+ * @param {Number} value The value to write.
+ * @param {Number} byteOffset The offset into the buffer to be written.
+ */
 function writeBufferComponentType(buffer, componentType, value, byteOffset) {


Why does this function need Component in it? Why not just writeBufferComponent? This writes one component to a buffer, right? The "type" doesn't need to be explicit in the name.

Same comment for readBufferComponentType.

pjcozzi · 2016-08-17T12:34:32Z

@lilleyse can you please scrub this public API to make sure the naming and general patterns follow Cesium's best practices? I could a few things with a quick look, but I suspect there are more from old code...not necessarily changes in this PR...but I want to get the initial public API as close to right as possible even if it won't be widely used for a while.

pjcozzi · 2016-08-17T12:35:31Z

@lasalvavida this is a huge step in the right direction, thanks for starting to kick this into shape!

lasalvavida · 2016-08-17T15:33:50Z

Updated, @lilleyse, this is ready for a look.

lasalvavida · 2016-08-17T15:49:44Z

Updated

lilleyse · 2016-08-17T19:39:31Z

I did some general cleanup which someone else should look over before I merge. The recent changes to index.js seem good to me.

lasalvavida · 2016-08-17T19:41:16Z

index.js

@@ -28,16 +26,18 @@ module.exports = {
    optimizeCache : require('./lib/optimizeCache'),
    packArray : require('./lib/packArray'),
    parseBinaryGltf : require('./lib/parseBinaryGltf'),
+    Pipeline : require('./lib/Pipeline'),


Is this correct? Shouldn't the capitalization put this first?

Oh, I didn't know that was a common thing to do. If so I'll correct it.

@mramato, I can't find a point of reference for this. Which way should this go?

It doesn't really matter in this case. Since this won't be auto-sorted, whatever you guys want to do is fine.

lasalvavida · 2016-08-22T13:58:02Z

Updated. AmbientOcclusion is bakeAmbientOcclusion, but the stage function is directly exported with the test functions exposed as private members.

lasalvavida · 2016-08-24T15:02:45Z

@lilleyse Is there anything else we need to do here or is this good to merge?

lilleyse · 2016-08-24T15:06:50Z

I think this is good to go.

Once we publish to npm we will have to remember to update several projects because of the gltfPipeline.js -> Pipeline.js name change.

lasalvavida added 2 commits August 10, 2016 13:41

Export all lib js as index.js

5524a73

Updated CHANGES.md

863be07

pjcozzi reviewed Aug 10, 2016
View reviewed changes

lasalvavida added 2 commits August 11, 2016 13:28

All exposed functions have defined headers for API

0f6298d

Removed index generation function

da16e7b

lasalvavida changed the title ~~Export all lib js as index.js~~ Export public API via index.js Aug 11, 2016

Updated CHANGES.md

ef85de6

lilleyse reviewed Aug 12, 2016
View reviewed changes

Small fixes

6b66d6f

lilleyse mentioned this pull request Aug 12, 2016

Added initial JSDocs for pipeline stages #132

Closed

lilleyse reviewed Aug 12, 2016
View reviewed changes

lasalvavida added 2 commits August 12, 2016 14:17

Add pipeline notes to the functions that need them

0b22358

Added reference doc for options

a880aab

Added computeFlatTransformScene pipeline note

8d43261

lilleyse reviewed Aug 12, 2016
View reviewed changes

lasalvavida added 5 commits August 16, 2016 16:33

Wrap objects with scope to have names for doc, remove

c58e5de

OptimizationStatistics

Tweaks

545fce1

Renamed nodeHelpersSpec to NodeHelpersSpec

c5bf895

Updated bin, CHANGES and README

4bf4745

Exclude doc from jsHint and jsdoc npm script

c752a1a

pjcozzi reviewed Aug 17, 2016
View reviewed changes

lasalvavida mentioned this pull request Aug 17, 2016

Roadmap #1

Closed

61 tasks

Made some function names and signatures more uniform, updated doc.

0c9d382

lasalvavida added 2 commits August 17, 2016 11:44

NodeHelpers should be private

5687191

Fixed doc error

d3c8e01

lilleyse force-pushed the export-all branch from b05cf47 to 7893b75 Compare August 17, 2016 19:29

Cleanup

3395a4f

lilleyse force-pushed the export-all branch from 7893b75 to 3395a4f Compare August 17, 2016 19:33

Rename optimizeCache to optimizeForVertexCache

a64d119

lasalvavida reviewed Aug 17, 2016
View reviewed changes

Expose test function as private members of bakeAmbientOcclusion

9f292b5

Missed a change

404401b

lilleyse merged commit 0aea48a into master Aug 24, 2016

lilleyse deleted the export-all branch August 24, 2016 19:33

This was referenced Aug 31, 2016

Exposing helpers #153

Closed

extensions.KHR_materials_common lights is left in even when the extension isn't used #158

Closed

Export public API via index.js #157

Export public API via index.js #157

Conversation

lasalvavida commented Aug 10, 2016 • edited Loading

lasalvavida commented Aug 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida commented Aug 11, 2016

lasalvavida commented Aug 11, 2016 • edited Loading

lasalvavida commented Aug 11, 2016

lasalvavida commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida commented Aug 12, 2016

lilleyse commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Aug 12, 2016

pjcozzi commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Aug 17, 2016

pjcozzi commented Aug 17, 2016

lasalvavida commented Aug 17, 2016

lasalvavida commented Aug 17, 2016

lilleyse commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasalvavida commented Aug 22, 2016

lasalvavida commented Aug 24, 2016

lilleyse commented Aug 24, 2016

lasalvavida commented Aug 10, 2016 •

edited

Loading

lasalvavida commented Aug 11, 2016 •

edited

Loading