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

Fix merge path handling #143

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Fix merge path handling #143

merged 4 commits into from
Jul 24, 2024

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jul 12, 2024

There have been some quirks in the handling of paths for the merge and mergeJson commands.

Some details are described in the conversation around a recent PR. An attempt to summarize some of the main points:

  • The merge command originally copied the (external) source tilesets to the target directory
  • It copied the sources into subdirectories of the target directory. The names of these subdirectories have been disambiguated with some not-so-well-thought-through "identifiers"...
  • For the mergeJson command, these "identifiers" (i.e. subdirectory names) did not make sense. The merged tileset JSON should just refer to the real source tileset JSON files, using relative URIs in the content.uri
  • The attempt to fix this unveiled some "skeletons in the closet". We had been aware of some of that, but ... maybe in denial about the implications (details below)

This PR changes these aforementioned tilesetSourceIdentifiers to actually be externalTilesetDirectories:

  • For the merge command, these still are the (disambiguated, but somewhat arbitrary) names of the subdirectories that the sources will be copied to
  • For the mergeJson command, these are the actual relative directories that will become part of the content.uri that refer to the source tilesets

@jo-chemla Maybe you want to give this a try. I tried to cover some of the relevant parts in the specs, differentiating the cases of [merge, mergeJson] ⨯ [directories, files]. But it could be worthwhile to consider adding "more tricky cases" there.


Details:

Most of the 3D Tiles Tools commands can transparently operate on 3tz or 3dtiles files, or on the file system. The latter generally allowed inputs and outputs to either be given as an explicit file name like "./example/tileset.json", or as a directory name like "./example", and then make the assumption that this directory contains a file called tileset.json. This is a leftover from CesiumGS/3d-tiles#184 . It becomes more tricky due to the fact that in 3tz and 3dtiles files, there is a requirement for the top-level tileset JSON to be called tileset.json. And conversely, when someone gives something like ./output as the target, it is by no means clear whether this should be a JSON file without an extension, or whether the output should be ./output/tileset.json 😬 I tried to sort some of that out, using 20 lines of comments for a 5 line function ...

@alouis-jpg
Copy link

Thank you for this update! It will save us some time.

Stepping in on behalf of @jo-chemla here, I ran all the tests listed in his PR and everything works as expected. Great work!

@jo-chemla
Copy link
Contributor

Thanks @alouis-jpg , indeed all 5 cases do work the same way the initial suggested PR did - mix of relative/absolute paths for input and output tileset-name-i.json listed in the table here extracted below:

inputs output Works? Children uri
absolute C:\tilesets\folderN\tilesetN.json absolute C:\tilesets\merged\tilesetMerged.json ../folderN/tilesetN.json
relative .\tilesets\folderN\tilesetN.json relative .\tilesets\merged\tilesetMerged.json ../folderN/tilesetN.json
relative .\tilesets\folderN\tilesetN.json absolute C:\tilesets\merged\tilesetMerged.json ../folderN/tilesetN.json
absolute C:\tilesets\folderN\tilesetN.json relative .\tilesets\merged\tilesetMerged.json ../folderN/tilesetN.json
absolute C:\tilesets\folderN\tilesetN.json absolute on other drive Z:\out\merged\tilesetMerged.json C:/tilesets/folderN/tilesetN.json

@lilleyse lilleyse merged commit b9ce25a into main Jul 24, 2024
2 checks passed
@lilleyse lilleyse deleted the fix-merge-path-handling branch July 24, 2024 14:48
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