Skip to content

Add support for optional changesets when loading iModels #12778

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.

Sign up for GitHub

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

Merged
ggetz merged 10 commits into from
Jul 31, 2025
Merged

Add support for optional changesets when loading iModels #12778

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2f8a6e5
Add support for optional changesets when loading iModels
r-veenstra Jul 30, 2025
993f73a
order of parameters, tests
r-veenstra Jul 30, 2025
7009961
update docs
r-veenstra Jul 31, 2025
e8cad43
update ITwinData to use options arguments
jjspace Jul 31, 2025
d339a71
do not request empty changeset ids
jjspace Jul 31, 2025
8bd3429
Merge remote-tracking branch 'origin/main' into imodel-changeset-support
jjspace Jul 31, 2025
3b25fd2
update changes
jjspace Jul 31, 2025
161fad8
Merge remote-tracking branch 'origin/main' into imodel-changeset-support
jjspace Jul 31, 2025
9974eb4
fix linting
jjspace Jul 31, 2025
ab4867f
update docs
jjspace Jul 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/engine/Source/Core/ITwinPlatform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,12 @@ ITwinPlatform.apiEndpoint = new Resource({
*
* @throws {RuntimeError} If the iTwin API request is not successful
*/
ITwinPlatform.getExports = async function (iModelId) {
ITwinPlatform.getExports = async function (iModelId, changesetId) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.string("iModelId", iModelId);
if (defined(changesetId)) {
Check.typeOf.string("changesetId", changesetId);
}
if (
!defined(ITwinPlatform.defaultAccessToken) &&
!defined(ITwinPlatform.defaultShareKey)
Expand Down Expand Up @@ -191,6 +194,9 @@ ITwinPlatform.getExports = async function (iModelId) {
if (typeof CESIUM_VERSION !== "undefined") {
resource.appendQueryParameters({ clientVersion: CESIUM_VERSION });
}
if (defined(changesetId)) {
resource.appendQueryParameters({ changesetId: changesetId });
}

try {
const response = await resource.fetchJson();
Expand Down
10 changes: 8 additions & 2 deletions packages/engine/Source/Scene/ITwinData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,19 @@ const ITwinData = {};
*
* @param {string} iModelId The id of the iModel to load
* @param {Cesium3DTileset.ConstructorOptions} [options] Object containing options to pass to the internally created {@link Cesium3DTileset}.
* @param {string} [changesetId] The id of the changeset to load, if not provided the latest changesets will be used
* @returns {Promise<Cesium3DTileset | undefined>} A promise that will resolve to the created 3D tileset or <code>undefined</code> if there is no completed export for the given iModel id
*
* @throws {RuntimeError} If no exports for the given iModel are found
* @throws {RuntimeError} If all exports for the given iModel are Invalid
* @throws {RuntimeError} If the iTwin API request is not successful
*/
ITwinData.createTilesetFromIModelId = async function (iModelId, options) {
const { exports } = await ITwinPlatform.getExports(iModelId);
ITwinData.createTilesetFromIModelId = async function (
iModelId,
options,
changesetId,
) {
Copy link
Contributor

@jjspace jjspace Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've been thinking back and forth on this. Especially with the recent (and somewhat lengthy) discussion around handling of arguments for this API as a whole in #12709

With optional arguments, there's an argument (😉) to be made that they should be ordered from most to least used. I also think it's valid to think about the "relation" between arguments. My gut says this should go ( iModelId, [changesetId], [tilesetOptions] ) as the imodel id and changeset id are the most related. However I think it's more likely that people will want to use the iModelId and the tilesetOptions and only rarely the changesetId which means the current arrangement is fine.

From the discussion in #12709 I almost think the best solution is ( iTwinOptions, [tilesetOptions] ) where the iTwinOptions is { iModelId, [changesetId] }. Or maybe even better a single ({ iModelId, [changesetId], [tilesetOptions] }) argument is actually best?

Future updates and "backwards compatibility" (even if this is experimental) are also worth taking into account. This order means all existing calls will "just work".

Open to some discussion on this (maybe thoughts from @javagl?). However if we need to include this for the release I'm ok with this solution and addressing all the functions with #12709

CC @ggetz

Copy link
Contributor Author

@r-veenstra r-veenstra Jul 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

To help add context to this decision, another parameter I would want to be able to define is a custom iTwin Share Key on a per tileset basis.

A common request from existing iTwin users is to be able to visualise multiple iTwins in one application. Currently a user can only define Cesium.ITwinPlatform.defaultShareKey which won't work if multiple iTwins are required.

It's also possible we would want to define a custom Cesium.ITwinPlatform.defaultAccessToken too if I'm using a different authentication method. And if the Mesh Export API that this wraps grows in capability, we'd want to easily support that in the future.

So I think the structure should lend itself to having a set of iTwinOptions that are easily expandable.

Copy link
Contributor

@jjspace jjspace Jul 31, 2025

Choose a reason for hiding this comment

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

We discussed this offline and decided to go with the route of a single options object for this API. I've updated this PR to change all the functions signatures (the core part of discussion in #12709) including the new changeset id option.

const { exports } = await ITwinPlatform.getExports(iModelId, changesetId);

if (
exports.length > 0 &&
Expand Down
11 changes: 11 additions & 0 deletions packages/engine/Specs/Core/ITwinPlatformSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,17 @@ describe("ITwinPlatform", () => {
expect(resource).toBeDefined();
expect(resource.url).toContain("imodel-id-1");
});

it("uses the changeset in the API request", async () => {
let resource;
requestSpy.and.callFake(function () {
resource = this;
return JSON.stringify({ exports: [] });
});
await ITwinPlatform.getExports("imodel-id-1", "changeset-id-1");
expect(resource).toBeDefined();
expect(resource.url).toContain("changeset-id-1");
});
});

describe("getRealityDataMetadata", () => {
Expand Down