mergeJson: Set merged tileset children uri to relative path to output directory #142
Conversation
|
I think that this was not really changed in the last PR. It was the behavior that the In any case, there certainly are different possible strategies for determining the URIs of the tilesets. And trying to set the URIs in the target based on the URIs of the input and the output directory (using the I'll have to carefully read the changes, think this through more thoroughly, and maybe do some further tests. For example, with absolute paths (even though it could be tricky to put that into unit tests...). |
|
Thanks for getting back again. This new implementation works as it should for all cases I could test it on - see table below. The difference between how merge and mergeJson should handle tilesets filepaths is that the simple This new implementation was tested on multiple iterations of absolute and relative inputs and outputs. Children uris always result in the correct relative filepath - only case when it is an absolute path is when input and output are on different drive letters. Could not update the spec to handle test-cases with absolute paths, hence I only added the test for relative input and relative outputs filepaths.
|
|
(Only a short comment - I still have to allocate more time for that:) I had seen the different handling of the "identifiers" (already linked to above, here in the last PR) that depended on the Some context: These "identifiers" are just used for disambiguating multiple input tilesets that "have the same name". This was not handled in the initial implementation of the The problem: The behavior of For example, the current (single) as the inputs, and tests that the resulting URIs for the tilesets will be These Now... with In the current state of the spec, you said Now, these content URIs are very long, very specific, and very dependent on the exact structure of the input and output. And it will make a crucial difference of whether a user has a directory structure like or From quickly looking over the table, it looks like the resulting
(you know, roughly like that...). The fact that this is so important (only) for the Sorry... so much about a "short comment" 😬 But I see these PRs as a chance to clarify some aspects of the behavior and describe it more strictly on a technical level. And I appreciate your feedback, contributions, and tests that appear to happen based on real-world examples, and not only one artificial example that is run as part of a unit test. |
|
Thanks for the "short comment", very much appreciated! It is now clearer to me, both the history behind the implementation and the use for these "identifiers" within the standard merge utility. I did understand indeed that these were made to disambiguate input tilesets directories named the same at two different locations, so they could be copied into output at the best location within output directory - as top-level as possible, with a suffix in case it is not the first children tileset directory named like that. In case of Hope this makes sense. In our workflow, this is the use-case we have, hence also testing it for all the different flavors of relative vs absolute input & output paths arguments to the CLI in the table above, to make sure all these cases are yielding expected results. |
|
All this makes sense. I think that the most common usage pattern for the and then just wants to create a new, top-level tileset, But now I realized that the differences in the internal handling of URIs for the The solution for the dismbiguation, using the But looking at the resulting core snippet, i.e. the loop in the for (const tilesetSourceName of tilesetSourceNames) {
// Determine the name of the file that contains the tileset JSON data
const tilesetSourceJsonFileName =
Tilesets.determineTilesetJsonFileName(tilesetSourceName);
// Determine an "identifier" for the tileset source
// (see `tilesetSourceIdentifiers` for details)
let tilesetSourceDirectoryPath;
if (Paths.isDirectory(tilesetSourceName)) {
tilesetSourceDirectoryPath = tilesetSourceName;
} else {
tilesetSourceDirectoryPath = path.dirname(tilesetSourceName);
}
const tilesetSourceDirectoryName = path.basename(tilesetSourceName);
const tilesetTargetDirectoryPath = Paths.isDirectory(tilesetTargetName)
? tilesetTargetName
: path.dirname(tilesetTargetName);
const tilesetSourceIdentifier = TilesetMerger.createIdentifier(
tilesetSourceDirectoryName,
this.tilesetSourceIdentifiers
);
const relativeTilesetIdentifier = path.relative(
tilesetTargetDirectoryPath,
tilesetSourceDirectoryPath
);
const tilesetSource = TilesetSources.createAndOpen(tilesetSourceName);
this.tilesetSources.push(tilesetSource);
this.tilesetSourceJsonFileNames.push(tilesetSourceJsonFileName);
this.tilesetSourceIdentifiers.push(
!jsonOnly ? tilesetSourceIdentifier : relativeTilesetIdentifier
);
}and squinting a little bit, it becomes apparent that these "identifiers" caused some quirks here: 1.: They sometimes are "arbitrary strings", and sometimes "relative paths". Meh. 2.: They require completely different information for their construction. This is somehow "hidden" by the for (const tilesetSourceName of tilesetSourceNames) {
// Determine the name of the file that contains the tileset JSON data
const tilesetSourceJsonFileName =
Tilesets.determineTilesetJsonFileName(tilesetSourceName);
const tilesetTargetDirectoryPath = Paths.isDirectory(tilesetTargetName)
? tilesetTargetName
: path.dirname(tilesetTargetName);
const tilesetSource = TilesetSources.createAndOpen(tilesetSourceName);
this.tilesetSources.push(tilesetSource);
this.tilesetSourceJsonFileNames.push(tilesetSourceJsonFileName);
if (jsonOnly) {
let tilesetSourceDirectoryPath;
if (Paths.isDirectory(tilesetSourceName)) {
tilesetSourceDirectoryPath = tilesetSourceName;
} else {
tilesetSourceDirectoryPath = path.dirname(tilesetSourceName);
}
const relativeTilesetIdentifier = path.relative(
tilesetTargetDirectoryPath,
tilesetSourceDirectoryPath
);
this.tilesetSourceIdentifiers.push(relativeTilesetIdentifier);
} else {
const tilesetSourceDirectoryName = path.basename(tilesetSourceName);
const tilesetSourceIdentifier = TilesetMerger.createIdentifier(
tilesetSourceDirectoryName,
this.tilesetSourceIdentifiers
);
this.tilesetSourceIdentifiers.push(tilesetSourceIdentifier);
}
}which more clearly shows their different construction. E.g when they are "arbitrary strings", there is no need for the 3. Most importantly: The previous PR, #140 , already did break the original (with the inputs all having the The reason is that the Again, all this boils down to questionable handling of the "identifiers". Apologies for not noticing the caveats here earlier. I'll try to create a fix for this ASAP. (This may be independent of this PR, but I'll certainly try to cover the use case and tests that you described). |
|
When running the new spec, then it expects the content URIs to be However, the directory structure within When I'm counting correctly, then there is one
Also, I kept the output files and served them with a local server, and the message The reason for that is probably that the |
|
Thanks for the detailed feedback, very much appreciated.
TLDR: completely agree with the proposed changes, these look indeed better! |
|
@jo-chemla I tried to address some of the issues in #143 . I tried to straighten out these "identifiers", and add some (maybe too) elaborate comments. Eventually, this could/should make this PR obsolete 🤞 |
|
Thanks for the feedback, great to hear. @alouis-jpg from our team did test against the same 5 cases of the table with a mix of absolute and relative paths for inputs and outputs (only on tileset files since we have no tileset named tileset.json) and they all yielded the same children uris with your PR. So this new PR #143 should be good to go! |
|
Based on your comments, this seems to now be resolved via #143 |
In the initial PR, I did not correctly set the children URI for the merged tileset. This PR makes it so that children URI is the relative path from the output directory to the respective input tilesets jsons filepaths.