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

Add crunch compressed texture support #4814

Merged
merged 8 commits into from
Jan 9, 2017
Merged

Add crunch compressed texture support #4814

merged 8 commits into from
Jan 9, 2017

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Jan 5, 2017

Note that this is a PR into #4758.

Any image with a .crn extension or a image/crn mime-type will be transcoded to DXTc on a web worker. Crunch supports a few formats, but only DXT1 RGB and DXT5 are supported here.

crunch.js was compiled to javascript with Emcripten. The compiler options are listed in the comments in crunch.js. The crn.cpp file is also needed. Right now I have it submitted to a fork. Where do we want to keep this file and the instructions for compiling?

@mramato
Copy link
Contributor

mramato commented Jan 6, 2017

Right now I have it submitted to a fork. Where do we want to keep this file and the instructions for compiling?

If we are going to have a permanent fork, it should probably be under the AnalyticalGraphicsInc user and we can just have the build instructions in the fork README. Might be a good idea to mention and link to the fork from one of our build/coding guides as well. Not sure if we have a section on ThirdParty libraries or not.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 9, 2017

If we are going to have a permanent fork, it should probably be under the AnalyticalGraphicsInc user and we can just have the build instructions in the fork README. Might be a good idea to mention and link to the fork from one of our build/coding guides as well. Not sure if we have a section on ThirdParty libraries or not.

+1 to everything above.

Here is the fork to use: https://github.com/AnalyticalGraphicsInc/crunch

* @param {Number} height The height of the texture.
* @param {Uint8Array} buffer The compressed texture buffer.
*/
function CompressedTextureBuffer(internalFormat, width, height, buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to CHANGES.md.

* @param {CompressedTextureBuffer} object The compressed texture buffer to be cloned.
* @return {CompressedTextureBuffer} A shallow clone of the compressed texture buffer.
*/
CompressedTextureBuffer.clone = function(object) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally add a member clone in addition to the static clone even if it isn't used in Cesium proper.

when) {
'use strict';

var transcodeTaskProcessor = new TaskProcessor('transcodeCRNToDXT', Number.POSITIVE_INFINITY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about Number.POSITIVE_INFINITY?

We have some bigger work to do on parallelism, but, in the meantime, what is a reasonable default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only option because anytime we load an image we expect a promise that will resolve to an image. Otherwise, we have to change that behavior so when undefined is returned the task is rescheduled. We use this option for geometry web workers as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, actually the request scheduler implicitly helps here. We'll flush this out later.

* @see {@link http://www.w3.org/TR/cors/|Cross-Origin Resource Sharing}
* @see {@link http://wiki.commonjs.org/wiki/Promises/A|CommonJS Promises/A}
*/
function loadCRN(urlOrBuffer, headers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplication among load* functions. Is it reasonable to implement them in terms of a @private load function that takes a callback?

/*global define*/
define([], function() {

// crunch/crnlib v1.04 - Advanced DXTn texture compression library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update LICENSE.md.

) {
'use strict';

// Modified from: https://github.com/toji/texture-tester/blob/master/js/webgl-texture-util.js
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update LICENSE.md.

}
}

function transcodeCRNToDXT(arrayBuffer, transferableObjects) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how we document workers, but I would explicitly mark this @private as we do elsewhere.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 9, 2017

Just those comments.

@bagnell
Copy link
Contributor Author

bagnell commented Jan 9, 2017

@pjcozzi This is ready for another look.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 9, 2017

Looks good! Also deleted the unnecessary fork.

@pjcozzi pjcozzi merged commit 304d32a into texture-compression Jan 9, 2017
@pjcozzi pjcozzi deleted the crunch branch January 9, 2017 22:40
@chris-cooper chris-cooper mentioned this pull request Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants