Add support for optional changesets when loading iModels

[r-veenstra]

Description

This PR resolves #12776 by adding support for an optional changesetId when loading iModels from iTwins using Cesium.ITwinData.createTilesetFromIModelId

Changesets define different versions of an iModel, and a common use case is to be able to load different versions and compare them side by side. The current implementation only supports loading tilesets from the latest changeset.

Note: Specifying an invalid changeset does not cause the Mesh Export API to error, instead it just returns an empty list of exports.

For discussion

• I originally have a check for empty exports and threw and exception, however the existing unit tests indicate that if no exports are present, returning an undefined tileset is the preferred outcome, so I have removed it for now.

• What order of parameters should Cesium.ITwinData.createTilesetFromIModelId have? I've currently implemented it as imodelId, tilesetOptions, changesetId so as to not break existing code. However, iModelId, changesetId then tilesetOptions seems more natural. Not sure on best practice here.

Issue number and link

#12776

Testing plan

I tested the following:

• Loading an iModel using existing methods, as per this sandcastle and verified it still works
• Loading an iModel using a valid changeset, confirming a different version appeared
• Loading an iModel with an invalid changeset string, confirming an empty list of exports was returned
• Loading an iModel with a non-string changeset, confirming an exception was thrown due to a non-string being used

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @r-veenstra! Welcome to the Cesium community!

In order for us to review your PR, please complete the following steps:

• Send in a Contributor License Agreement (CLA)
• Add yourself to the contributors file

Review Pull Request Guidelines to make sure your PR gets accepted quickly.

[ggetz]

Thanks @r-veenstra! I can confirm we have a CLA on file covering you. 🙂

@jjspace Could you please review?

[jjspace]

First, whatever we settle on please make sure to update the CHANGES.md file with this change.

Second, to give some historical context. In the original PR that implemented this API, #12289, we specifically decided to not include support for specifying a specific changeset (commit where I removed it). At the time it was my understanding that a vast majority of users would never need or want to request anything but the latest version of an iModel. This was based off discussion with @danieliborra.
To be clear, I'm not against including it I just want to make sure it's very intentional why we're including it. If this is to support a tutorial that's also highlighting the "best practices" and intended use cases for the iTwin platform is this actually the desired workflow?

[jjspace]

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

[r-veenstra]

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.

[jjspace]

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.

[jjspace]

Thanks for the updates @r-veenstra.

As stated above we've decided to push ahead with the changes to the API signature for all functions in the ITwinData class. I've made these changes on this branch. There is a fallback that will last for 1 version to attempt to detect the old signature and use those arguments. (I'll be opening a PR to remove this shortly that should be merged next week after the release goes out.)
This addresses half of #12709, the tileset options will be added in a follow up PR but will be much easier with this current change.

@ggetz would you be able to review my changes so there's a second pair of eyes on them? I think it should be good to merge if you're happy.

CC @javagl

[danieliborra]

First, whatever we settle on please make sure to update the CHANGES.md file with this change.

Second, to give some historical context. In the original PR that implemented this API, #12289, we specifically decided to not include support for specifying a specific changeset (commit where I removed it). At the time it was my understanding that a vast majority of users would never need or want to request anything but the latest version of an iModel. This was based off discussion with @danieliborra. To be clear, I'm not against including it I just want to make sure it's very intentional why we're including it. If this is to support a tutorial that's also highlighting the "best practices" and intended use cases for the iTwin platform is this actually the desired workflow?

This is a technical problem for a few reasons:

• We are not generating tilesets for all changesets (and we won't)
• We only keep the latest 5 exports and delete the rest (e.g. one of the iModels has +150K changesets, and it is not a small one, imagine generating and storing 150K tilesets).
• You can manually generate changesets, but it generates a lot of confusion even with Bentley colleagues/products.
• ChangesetIds are available before tileset generation, so, depending from where you get the changesetId, you may not have the tileset yet.

So, my strong advice would be not to do it, especially in a tutorial.

PS. I did some calculations, and storing +150K tilesets (3GB), would cost $9,450 per month.

[ggetz]

Thanks for the updates @jjspace, everything looks good. Just one comment on the changelog.

[ggetz]

Thanks for the insight @danieliborra.

We'll discuss with @r-veenstra on whether to move forward or not given this info. If we do end up moving forward, it sounds like at minimum, some additional context or disclaimers in the ref doc would be helpful for users.

[jjspace]

This should be good to go but CI keeps failing on the same test, I'm not sure why any change in this PR would affect the Cesium3DTileset specs... That test passes on my system, @ggetz thoughts?

[ggetz]

This should be good to go but CI keeps failing on the same test, I'm not sure why any change in this PR would affect the Cesium3DTileset specs... That test passes on my system, @ggetz thoughts?

I'm pretty much positive that's unrelated to these changes. Perhaps something became more fragile from a recent change. But test are passing in CI on the last run, and everything is passing locally for me.

[danieliborra]

Just to complete my explanation, one changeset is not a file version but a list of transactions, that is why you can get so many changesets and why we don't generate a tileset per changeset.

Said that, I checked the tutorial and it explains it very well. So, my hesitations are gone 👍