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

Obtain Base Url for Texture Requests within Reality Tile Loader #7450

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

andremig-bentley
Copy link
Contributor

@andremig-bentley andremig-bentley commented Dec 5, 2024

Currently, there is a bug attaching a 3dTiles tileset to the itwin viewer using attachRealityModel where if that model has textures, and the tileset has relative URIs for those textures, they will not be requested. This is because the base url for those requests was not being passed along to the gltf reader, and so an attempt to resolve the images for these textures would fail. This PR addresses that issue by creating the new optional baseUrl parameter passed down through the creation of the RealityTileTree. baseUrl is created using the new getTilesetUrlFromTilesetUrlImpl in RealityDataSource, which obtains the tileset url from a source which is an instance of RealityDataSourceTilesetUrlImpl. In RealityTileLoader.ts, the url is checked to determine if it's a valid url, and if so, it is passed in to the creation of the GltfReaderProps, and therefore, to the gltf reader.

@aruniverse
Copy link
Member

does this need to be backported to 4.10? or can it go in 4.11?

const treeId = tile.tree.id;
const split = treeId.split(":");

/* Splitting with ":" will end up splitting up our base url as well as the id.
Copy link
Member

Choose a reason for hiding this comment

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

all of this seems very fragile,
please add unit test for these

Copy link
Contributor

Choose a reason for hiding this comment

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

@aruniverse does the added unit test suffice?

@andremig-bentley
Copy link
Contributor Author

does this need to be backported to 4.10? or can it go in 4.11?

No backport is needed, 4.11 is fine, thanks.

@andremig-bentley
Copy link
Contributor Author

An update on this PR - Due to the fragility of the tree id parsing method, the methods of obtaining the base url for this PR have changed. Now, all tree id parsing has been removed, and instead, the id has been passed down through the creation of the RealityTileTree as the new optional member rdSourceId. By passing down the id directly, we avoid any possible issues that would come with parsing and avoid errors that might have been caused by unexpected aspects of the tree id.

Tests have been added to a new file, RealityTileLoader.test.ts, which test the creation of GltfReaderProps including or not including a base url as expected. This new test file includes a lot of set up code from GltfReader.test.ts which was necessary to successfully create GltfReaderProps. In my opinion, these tests make more sense being in their own file rather than being in GltfReader.test.ts, but they can be moved/changed if necessary.

Finally, we were able to test the case where a base url is passed in to the gltf reader for a tileset with full texture urls - the case discussed in my comment above. The texture requests worked as expected with no issues, so that edge case is a non-issue.

/** Returns the source's tileset url if the source is an instance of RealityDataSourceTilesetUrlImpl.
* @internal
*/
export function getTilesetUrlFromTilesetUrlImpl(rdSource: RealityDataSource): string | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Delete this, add an optional method on the RealityDataSource interface, and implement it in RealityDataSourceTilesetUrlImpl. That will allow any other implementations of RealityDataSource to implement it themselves if they so choose.

return glbFromChunks(chunks, header);
}

describe("createReaderPropsWithBaseUrl", () => {
Copy link
Member

Choose a reason for hiding this comment

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

These tests don't prove much. A much more valuable test would confirm that you successfully resolve a resource inside the glTF with a relative URL where prior to your change you would have failed to do so.

andremig-bentley and others added 2 commits December 18, 2024 08:25
Co-authored-by: Paul Connelly <22944042+pmconne@users.noreply.github.com>
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.

4 participants