Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup #130

Closed
5 of 8 tasks
lilleyse opened this issue Jul 11, 2016 · 18 comments
Closed
5 of 8 tasks

Cleanup #130

lilleyse opened this issue Jul 11, 2016 · 18 comments
Labels

Comments

@lilleyse
Copy link
Contributor

lilleyse commented Jul 11, 2016

Here is a rough list of tasks for bringing this repo up to production. Feel free to edit or add new items to the list:

  • We should have a common set of models that we use for testing new features, especially models that have created problems in the past. However I'm not sure if they should be hosted in this repo.
  • Refactor gltf-pipeline as a series of stages with dependencies
  • General cleanup, api standardizing, file renaming, folder organization
    • Many functions expect an options object. Instead of sending in {} in a lot of places, just do it the Cesium way and create the options object if it's not defined with options = defaultValue(options, defaultValue.EMPTY_OBJECT);
  • Use recommended libraries, Review npm dependencies #178
  • Use JSDoc everywhere
  • Progress events: Progress events #92
  • Something like jQuery for glTF to eliminate redundant glTF data structure traversal
  • Note: If there are any changes to the api we will need to update OBJ2GLTF and the website model converter
@lasalvavida
Copy link
Contributor

Added a list of functions that are considered "stages" and should be converted to use promises in #99.

@JoshuaStorm
Copy link

We use mkdirp in a handful of files, which the Node guide encourages be replaced with fs-extra's ensureDir and ensureFile. Should this be added to the cleanup?

@lasalvavida
Copy link
Contributor

We use mkdirp in a handful of files, which the Node guide encourages be replaced with fs-extra's ensureDir and ensureFile. Should this be added to the cleanup?

Yes

@JoshuaStorm
Copy link

Oh, I guess that is included in the

Use libraries recommended in the node guide

my bad

@lasalvavida
Copy link
Contributor

lasalvavida commented Jul 14, 2016

We should have a common set of models that we use for testing new features, especially models that have created problems in the past. However I'm not sure if they should be hosted in this repo.

@lilleyse What do you think about including the glTF sampleModels as a git submodule?

@lilleyse
Copy link
Contributor Author

I'm worried that may bloat the size of this repo.

@lasalvavida
Copy link
Contributor

If it's linked via submodule, don't the files only get pulled if you git submodule --init?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 14, 2016

I would if we could publish the sample models to npm. Is that a legit npm use case since it is not code? We might have to hold on this for a bit so Khronos can organize the models.

@JoshuaStorm
Copy link

JoshuaStorm commented Jul 14, 2016

  • Consider changing the default output directory name for the createDirectory flag. Currently it just defaults to creating a new directory named output within the base of the output path

@lasalvavida
Copy link
Contributor

I would if we could publish the sample models to npm. Is that a legit npm use case since it is not code?

Looks like probably not.

From https://www.npmjs.com/policies/conduct#acceptable-package-content:

Packages published to the Service must be created using the npm command-line client, or a functionally equivalent implementation. For example, a "package" must not be a PNG or JPEG image, movie file, or text document. Using the Service as a personal general-purpose database is also not allowed for this reason. Packages should be npm packages, and nothing else.

While 3D model files aren't explicitly prohibited here, I think the gist is that npm isn't for data.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 19, 2016

While 3D model files aren't explicitly prohibited here, I think the gist is that npm isn't for data.

Yes, agreed, but I wonder if there is an exception for data used for unit tests. Can you look into it? Maybe email them or ask on a forum?

@lasalvavida
Copy link
Contributor

To whom it may concern,

I had a quick question about npm's policy on data for unit tests. Over at https://github.com/AnalyticalGraphicsInc/gltf-pipeline, we are looking to add unit tests for a set of sample models. We were wondering if it would be within the terms of use to have an npm module for providing those test models. This would also be useful for tests for other glTF utilities and renderers.

I understand that npm generally doesn't allow this type of usage as in "Service as a personal general-purpose database is also not allowed for this reason", but wasn't sure if this still applied when it was being used as test data for other node modules.

Thank you,
Rob

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 31, 2016

@lasalvavida did you ever get a response from npm?

@lasalvavida
Copy link
Contributor

No response from npm. A git submodule for the new gltf-sample-models repo is probably the way to go on this one.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 31, 2016

I wouldn't jump on that yet since we want some of those models, but not all of them, and we'll also want some of our own.

@emackey
Copy link

emackey commented Feb 23, 2017

Just my $0.02. The git submodule strategy mentioned above sounds fine. Clients who want to run unit tests will need disk space for a copy of the sample model repo, but the size of the sample model repo doesn't "bloat" the size of this repo itself. This repo just gets a hash and a reference to the submodule. This means:

  • You always have the option to delete it in the future, leaving behind only a history of 1-line hashes.
  • Any clients who want to clone and run the pipeline without getting the sample models can do so simply by not running the git submodule commands when they clone/fetch/pull.

@ggetz
Copy link
Contributor

ggetz commented Jun 25, 2018

@lilleyse OK to close? Added cleanup label to outstanding issues. Maybe open one for tree traversal?

@lilleyse
Copy link
Contributor Author

Oh yes, this is pretty old now. ForEach pretty much fixed the traversal item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants