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

Uniform function templates #99

Closed
11 tasks
lasalvavida opened this issue Jun 20, 2016 · 7 comments
Closed
11 tasks

Uniform function templates #99

lasalvavida opened this issue Jun 20, 2016 · 7 comments
Labels
cleanup enhancement good first issue An opportunity for first time contributors

Comments

@lasalvavida
Copy link
Contributor

lasalvavida commented Jun 20, 2016

As we continue adding pipeline stages, it is going to be important to keep a uniform template for pipeline stage functions so that they can be used in the same way.

I would propose the following:

/**
 * Performs some operation on a glTF hierarchy.
 *
 * @param {Object} gltf An object holding a glTF hierarchy.
 * @param {Object} [options] Defines more specific behavior for this stage.
 *
 * @returns {Promise} A promise that resolves when the stage completes.
 */
function stage(gltf, options) {
    ...
}

For #130:
I would classify the following as "stages" and should be converted to use this template. Anyone can feel free to do them in any order; it doesn't need to be one big pull request.

  • addDefaults
  • removeUnused (and all substages of removeUnused)
  • mergeDuplicateVertices
  • removeUnusedVertices
  • mergeDuplicateAccessors
  • removeDuplicatePrimitives
  • convertDagToTree
  • combineMeshes
  • cacheOptimization
  • quantizedAttributes
  • encodeImages
@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 21, 2016

Agreed. I suggested this here: #75 (comment)

For an example in Cesium, see https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/Cesium3DTileContent.js

@JoshuaStorm
Copy link

I can start going through some of our unlabeled functions.

@lilleyse, thoughts on @lasalvavida's template before I move forward?

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jun 21, 2016

@JoshuaStorm We probably want to go the whole way with @pjcozzi's suggestion. That is, make a Stage.js that looks something like this:

function Stage() {
    this.name = undefined;
    this.before = [];
    this.after = [];
};

Stage.prototype.run = function(gltf, options) {};

Then change the stages that currently just export functions to follow the Stage template.

module.exports = RemoveUnusedMeshesStage;

function RemoveUnusedMeshesStage() {
    this.name = 'removeUnusedMeshes';
    this.before = ['addPipelineExtras'];
    this.after = [];
}

RemoveUnusedMeshes.prototype.run = function(gltf, options) {
    return new Promise(resolve, reject) {
        // This is a synchronous stage, so it's pretty easy
        // async will need a little more work, but shouldn't be too difficult
        removeUnusedMeshes(gltf);
        resolve();
    }
}

The function bodies won't need to change until we have something set up for using the before and after to resolve dependencies, but it would be good to go ahead and set them up while we're making this change.

@lasalvavida
Copy link
Contributor Author

We may also want to start keeping stages in a subdirectory of lib and utility functions in another.

@lilleyse
Copy link
Contributor

Yup, all the suggestions here should be part of a larger cleanup/restructuring process, which I'm going to open an issue for pretty soon and @lasalvavida will handle.

@JoshuaStorm in the meantime there are a few other stages you can work on like pre-cache vertex optimization and transparent textures.

@lilleyse lilleyse mentioned this issue Jul 11, 2016
8 tasks
@JoshuaStorm
Copy link

@lasalvavida What exactly is the purpose of having a synchronous function return as a promise?

@lasalvavida
Copy link
Contributor Author

@JoshuaStorm It is so that all stages can be executed in the same way. Eventually, when we have stage dependencies and custom pipelines we need to be able to run a stage by name without knowing anything about it and ideally without needing to do any special handling for async stages.

However, I think it actually makes more sense to have synchronous stages return undefined and asynchronous ones return promises which would be equally easy to handle, but would result in fewer changes to the code-base.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup enhancement good first issue An opportunity for first time contributors
Projects
None yet
Development

No branches or pull requests

5 participants