added image processing extras

[likangning93]

In anticipation of ambient occlusion and other future image processing stages, I added extras for use with jimp (https://www.npmjs.com/package/jimp) in the form of a gltf-level scratch Jimp image and Jimp copies of all the existing images, stored as jimpImage in extras._pipeline.

Since this won't be used for every pipeline configuration, I tried to make it as optional as possible.

I also added relevant specs.

[coveralls]

Coverage decreased (-0.4%) to 93.806% when pulling e181981 on loadGltfUris_jimp into 0a06110 on master.

[lilleyse]

I'm a bit torn, because while it's faster to make the image processing an optional step, it makes the code a bit more confusing. It would be kind of nice if image processing was completely hidden from gltfPipeline.js, and instead it always decoded in loadGltfUris and encodes in writeSource (but only if the data was changed, this can be marked by a flag in the image's extras that is set by the AO stage/atlas stage etc).

An alternative is to continue to allow readGltf and loadGltfUris to take the imageProcess flag, but not have an encodeImages stage in gltfPipeline, instead always encode in writeSource using the flag idea above.

Let me know what you think, or if I'm overlooking something.

[pjcozzi]

Can't this be handled with each stage declaring its dependencies? See #75.

[likangning93]

I really like option 2 for re-encoding. I also toyed with the idea of
changing 'loadGltfUris' to take an options parameter instead of 'basePath',
which might let us hide the image processing more. I'll give it a try.

On Thursday, June 16, 2016, Sean Lilley [email protected] wrote:

I'm a bit torn, because while it's faster to make the image processing an
optional step, it makes the code a bit more confusing. It would be kind of
nice if image processing was completely hidden from gltfPipeline.js, and
instead it always decoded in loadGltfUris and encodes in writeSource (but
only if the data was changed, this can be marked by a flag in the image's
extras that is set by the AO stage/atlas stage etc).

An alternative is to continue to allow readGltf and loadGltfUris to take
the imageProcess flag, but not have an encodeImages stage in gltfPipeline,
instead always encode in writeSource using the flag idea above.

Let me know what you think, or if I'm overlooking something.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#91 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AFJTN9BRgESGRtXNUIqnPzTutqOSCaWkks5qMckYgaJpZM4I3xQl
.

[likangning93]

Updated! As discussed offline, we decided to keep the encodeImages stage where it is for now. It seems like writeSource (which handles moving extras back into the json) is currently tied to writeGltf and not to writeBinaryGltf, and we may want to move it to come before the actual file writing options (#93). When that happens we will move encodeImages over to that. encodeImages also now checks for an imageChanged flag before encoding.

I also updated loadGltfUris to take in an options parameter instead of just basePath and updated all relevant specs accordingly.

[likangning93]

merged in master.

[coveralls]

Coverage decreased (-0.4%) to 93.816% when pulling 438f84c on loadGltfUris_jimp into 0a06110 on master.

[lilleyse]

Fix the ordering here.

[likangning93]

Updated!

[coveralls]

Coverage decreased (-0.4%) to 93.854% when pulling f6d4f01 on loadGltfUris_jimp into 0a06110 on master.

[likangning93]

@pjcozzi integrating image processing into runStages might be a little messy since the encoding and decoding have to happen at the beginning and end of the pipeline and it seems like runStages handles the stages that are more synchronous.

I imagine we could add something before runStages that checks if the pipeline needs image processing extras, but as @lilleyse pointed out offline that could get messy if we have to add anything else that wraps the pipeline like this.

[lilleyse]

There are a few lines like this that aren't used. Otherwise this PR looks ready to me.

[likangning93]

ok. Updated!

[coveralls]

Coverage decreased (-0.4%) to 93.854% when pulling 364c450 on loadGltfUris_jimp into 0a06110 on master.

[lasalvavida]

@pjcozzi integrating image processing into runStages might be a little messy since the encoding and decoding have to happen at the beginning and end of the pipeline and it seems like runStages handles the stages that are more synchronous.

We should adopt a promise style architecture for both synchronous and asynchronous stages; it would allow all stages to be run in the same fashion.

[pjcozzi]

We should adopt a promise style architecture for both synchronous and asynchronous stages; it would allow all stages to be run in the same fashion.

Yes, please add it to #1.

[pjcozzi]

} else {