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 static loadJson method to Cesium3DTileset #5685

Merged
merged 8 commits into from
Aug 11, 2017

Conversation

jason-crow
Copy link
Contributor

This pull request adds a hook to Cesium's Cesium3DTileset's method for requesting the tileset json. The purpose of this is to make it easier to extend or decorate the method a tileset uses to fetch its json. For instance this facilitates fetching json's from remote servers that may require changing the base url or providing authentication headers. These are use cases that are essential to Bimium's use cases for creating tilesets with the Cesium library.

It doesn't seem necessary at the moment to also provide a hook for the TileContent request for the binary file, since this specific type of request is marked by Cesium.RequestType.TILES3D, it is already easily overridden without any specific hooks provided.

I provided 2 tests cases, the first just tests that Cesium3DTileset.loadJson by default loads json, the second test is more applicable to my use case for creating it. It tests to ensure the Cesium3DTileset.loadJson is used in the constructor for fetching the tileset json, as this is the assumption that makes decorating/extending it useful. To prove this it override the loadJson function and replaces the passed url which is invalid in the test with a valid one and tests whether the readyPromise resolves successfully to prove that overriding this method is useful for altering the method for fetching the tileset's json.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 28, 2017

Thanks @jason-crow! @lilleyse will be the best person to review this. It might take a bit as we are focused on prepping for SIGGRAPH next week.

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Looks good!

Cesium3DTileset.loadJson = function(tilesetUrl)
{
return originalLoadJson(path);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix formatting:

Cesium3DTileset.loadJson = function(tilesetUrl) {
    return originalLoadJson(path);
}

@@ -1049,6 +1049,14 @@ define([
});

/**
* Provides a hook to override the method used to request the tileset json
* useful when fetching tilesets from remote servers
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Add @param and @returns to the documentation.

@jason-crow
Copy link
Contributor Author

Updated for the 2 changes you recommended. Should be ready to look at again.

@@ -1049,6 +1049,16 @@ define([
});

/**
* Provides a hook to override the method used to request the tileset json
* useful when fetching tilesets from remote servers
* @param {String} tilesetUrl, the url of the json file to be fetched
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comma and capitalize the

* Provides a hook to override the method used to request the tileset json
* useful when fetching tilesets from remote servers
* @param {String} tilesetUrl, the url of the json file to be fetched
* @returns {Promise<Object>} A promise that resolves with the fetched json data
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this needs to be Promise.<Object>

@jason-crow
Copy link
Contributor Author

Done

@lilleyse lilleyse merged commit 48ff4ba into CesiumGS:master Aug 11, 2017
@lilleyse
Copy link
Contributor

Thanks @jason-crow

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 14, 2017

Thanks again @jason-crow!

@lilleyse should you update CHANGES.md in master?

@lilleyse
Copy link
Contributor

Yes, added here: cce2392

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