CesiumGS / 3d-tiles-tools

Apache License 2.0
307 stars 46 forks source link

[Feature Request] allow for `merge` Tileset utility to only produce root tileset json and avoid tile contents copy #44

Closed jo-chemla closed 4 months ago

jo-chemla commented 1 year ago

Using the merge utility as presented in the docs starts by producing an output tileset.json root file which references the input tilesets in children directories. Then it continues by copying all the tile contents to the output folder.

npx 3d-tiles-tools merge -i tileset_1/n_0.json -i tileset_2/n_0.json -o ./_merged_tileset

Since there are lots of files to copy, this copy operation can take a lot of time. Having an option to disable tiles copying (json + b3dm) would be great! Therefore, the output tileset would only reference the input tilesets - probably via a uri relative to the output tileset file. eg in the case above, the output tileset would look like

{
  "asset": {"version": "1.1"},
  "geometricError": 1,
  "root": {
    "boundingVolume": {...},
    "refine": "ADD",
    "geometricError": 1,
    "children": [
      {
        "boundingVolume": {...},
        "content": {
         // Important line: relative reference to merged tilesets
          "uri": "../tileset_1/n_0.json"
        },
        "geometricError": 1,
        "refine": "REPLACE"
      },
     // ...
    ]
  }
}

Note: this might be what the combine utility is made for, but since it does not process in my case because of a Content does not have a URI error (properties are url), I could not check, see this feature request #43

javagl commented 1 year ago

This certainly could make sense for some cases. We also noticed that people occasionally want to "merge" tilesets, but expect the result to be what could be achieved by first calling merge and then (additionally!) calling combine.

(And this should yield the right result in your case, if the input tilesets are "upgraded" to use uri instead of url, or we change the code to be able to handle this difference transparently - details in the linked issue).

There are some corner cases to consider here:

Right now, most of the functionality of the 3d-tiles-tools is targeted at being able to transparently operate on the file system or on 3D Tiles package files (i.e. .3dtiles or .3tz files). Very roughly: You could run

merge -i tilesetA.3dtiles -i tilesetA.3dtiles -o ./merged.3dtiles

and receive a .3dtiles file that contains all the tiles from the input data (and it has to contain them to be "complete").

But there probably are some possible solutions for that:

jo-chemla commented 1 year ago

Hi Marco, thanks a lot for the detailed feedback! Understood for the current requirement to support input/output tilesets which would either be read/written to the filesystem or self-sustained, containerized archive formats - hence the need to copy over the tile contents as well.

In that case, the first createMergedJson option is probably better workflow-wise - it would avoid copying the tile contents before combining the tilesets, which can take a lot of time for large tilesets. If I understand the difference between merge and combine, then I'm not exactly sure why people would want to call them subsequently in that order.

For ref, we have produced a few hundreds 3D-scans (heritage sites, monuments and cities) at Iconem over the years, each consisting of poly-counts from a few hundreds of millions to a few billions primitives (polygons/points), largest being a complete multi-resolution Venice at 15B points.

jo-chemla commented 4 months ago

createMergedJson would be a perfect fit for our use case - we handle tile content files operations but want to create a mega-tileset that merges several tilesets which would stay each in place and be referenced via the uri property, rather than being copied to the output tileset directory. After digging a bit in the codebase, it seems that adding a boolean flag like json-only to the 3d-tiles-tools merge command, we could then simply check for the value of that flag before deciding to proceed or not to execute this.copyResources(); in TilesetMerger.ts https://github.com/CesiumGS/3d-tiles-tools/blob/ccbfbdc4281e243dbdf7e4b12408cc86db8afda2/src/tools/tilesetProcessing/TilesetMerger.ts#L195

Would you be open to a PR handling that use case?

Side-question: I have a tileset I created using merge on two distinct and partly overlapping tilesetA and tilesetB. Each tileset being tiled mesh has the refine: REPLACE policy setup which is expected. But setting the refine policy to either ADD (default) or REPLACE on the merged tileset, do always result in cesium-js viewer loading both overlapping tiles from both tilesets. Is this expected because tilesets do not share the same bounding volumes geometries?

Thanks for your help!

javagl commented 4 months ago

Yes, sorry that there has not been any progress here. It could be a relatively low-hanging fruit in terms of the implementation effort.

After digging a bit in the codebase, it seems that adding a boolean flag like json-only to the 3d-tiles-tools merge command, we could then simply check for the value of that flag before deciding to proceed or not to execute this.copyResources(); in TilesetMerger.ts

A bit of context: As mentioned above, the operations like merge usually work on the file system, 3DTILES files, or 3TZ files transparently. The core functionality of "creating the merged tileset JSON" could probably be implemented in a simpler way. For example, this block could be extracted into a function that just does "Tileset[] in - Tileset out". But that's probably not necessary for just offering the desired functionality here.

Would you be open to a PR handling that use case?

Certainly. There are not really any detailed instructions for contributors, but smaller things (like formatting or updating the change log) could be done separately.

In any case, you will have to sign the CLA at https://github.com/CesiumGS/cesium/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla

Based on the existing structure, your suggestion of a boolean flag to skip the copyResources call would certainly be a reasonable approach. That's the level of the implementation. Beyond that, there are a few degrees of freedom for how this should be offered at the CLI. Roughly:

I'm somehow slightly leaning towards the latter (roughly, favoring "self-contained" commands over ones that have (eventually 'many') parameters for configuration). But I don't have a strong opinion. What do you think?

(BTW: If you don't want to take this up, I can allocate a bit of time for implementing that in the next days)

jo-chemla commented 4 months ago

Thanks for that feedback, very much appreciated! Just submitted a PR here https://github.com/CesiumGS/3d-tiles-tools/pull/140 (+ signed the CLA). This follows the first bullet point as it was simpler for an outsider to the codebase, but I can modify the PR to add the glue-code around creating a dedicated mergeJson command. I don't have a strong opinion on that matter either, you know the project better so I'll follow your advice.

javagl commented 4 months ago

From quickly skimming over the changes, the description (at the CLI and the params) seems to be reversed...

@param jsonOnly - Whether to copy resources to output directory

It says whether to not copy the resources (when it is true, then the resources are not copied). Maybe something like "Whether only the merged tileset JSON should be generated" (or so)... ? But ... I'll try it out first and see whether there is any further feedback.

jo-chemla commented 4 months ago

Indeed, thanks. I went ahead and followed your initial suggestion, splitting merge and mergeJson into 2 distinct CLI methods and puhsed to the PR.

I also tried forcing line-endings to LF (rather than CRLF on my windows machine in the .prettier.json file as well as modifying the prettier command from the package.json to "prettier --end-of-line lf --write \"**/*\"" but no luck so not completely sure line endings are correct on my modified files.

Hope this helps, and again thanks for the directions! (+ I can revert to the having the jsonOnly flag to merge if that's a best fit).

{
    "endOfLine": "lf"
}
javagl commented 4 months ago

Fixed via https://github.com/CesiumGS/3d-tiles-tools/pull/140 , thanks @jo-chemla !

jo-chemla commented 4 months ago

Hi there, just made a new PR since I found a better way to handle relative paths from output directory to each input tileset filename, so output merged tileset.json produced via mergeJson can be consumed directly in place. Here is the link to the PR: https://github.com/CesiumGS/3d-tiles-tools/pull/142 !