CesiumGS / 3d-tiles-tools

Apache License 2.0
289 stars 43 forks source link

`glTF model references separate files but no resourceDirectory is supplied` Error when upgrading legacy 3d tiles #109

Open Insopitus opened 6 months ago

Insopitus commented 6 months ago

Command: 3d-tiles-tools upgrade -i ./tiles/tileset.json -o output -f Error: ERROR (upgrade): Failed to upgrade Tile_+000_+000_+000_L20_00000.b3dm: RuntimeError: glTF model references separate files but no resourceDirectory is supplied

test tiles

I tried to read the source code and found out the error is thrown by gltf-pipeline code. The options param doesn't have a resourceDirectory field so the program don't know where to find the separated shader and image files. I don't know how to fix it though.

javagl commented 6 months ago

A few first analysis steps:

1. The error message is indeed coming from gltf-pipeline. The B3DM files of the input contain to glTF 1.0 (GLB) files. The gltf-pipeline is used to update these to glTF 2.0 (involving some guesses and best-effort steps...). Now, the structure of the tileset here is a bit unusual: The GLB files refer to external textures (which are given as JPG files). Usually, these should be contained in the GLB itself.

However, this can be solved by passing in the resourceDirectory to the upgrade command. (The options are untyped and not documented extensively, but ... you know, JavaScript, you can throw in whatever you want). So it should be possible to perform the upgrade with a call like this: 3d-tiles-tools upgrade -i ./input/tileset.json -o ./output/tileset.json -f --options --resourceDirectory ./input/ (Note the --resourceDirectory ... part that basically points to the place where the JPG files can be found)

2.. But...

There seems to be something wrong with one of the input B3DMs. All of them seem to use a really old legacy version of B3DM. And that's ... "ok-ish". The upgrade should be capable of handling exactly that. But the Tile_+000_+000_+000.b3dm file seems to be "invalid/broken" in some way (even for the legacy format). I have not yet analyzed this in detail. It does not seem to be used in the tileset.json after all - maybe it can just be deleted? Otherwise, I'd have to dive into that, and see whether its contents could be salvaged...

3. The bounding volume is waaay off

Loading the original (legacy) tileset in CesiumJS and showing the bounding volume gives this:

Cesium tools issue 109 image 01

The fact that the content is not contained in the bounding volume will cause all sorts of rendering glitches, eventually. And ... this is not fixed via the upgrade process either.

There are some potential options for fixing the bounding volumes. But for now, this would only be a quick workaround for something that could/should be addressed more holistically via something like https://github.com/CesiumGS/3d-tiles-tools/issues/106

lilleyse commented 6 months ago

@javagl is it possible for 3d-tiles-tools to internally pass through resourceDirectory to gltf-pipeline?

javagl commented 6 months ago

EDIT: Maybe I misunderstood this. Maybe you meant to always pass the resourceDirectory to gltf-pipeline, regardless of whether it was given at the command line? That could be doable as well, with some care about input/output being 3DTILES/3TZ and such. I can have another look...


@lilleyse It does pass through the resourceDirectory (or rather: all options) for the standard upgrade command. That was point 1. from the answer above, and this should already solve the main part of this issue.

It does not (yet?) pass it through when the targetVersion=1.1 is set, because it takes a different path there - roughly: migrating instead of (only) upgrading. There, it handles the case of glTF 1.0 only with a "last minute" call to the gltf-pipeline (just before obtaining the glTF-Transform Document). So the resourceDirectory could be passed through there as well. It wouldn't be sooo nice, because it's either these (untyped) options:any or the hard-to-explain resourceDirectory:string that has to be passed through several methods, but I'll have another look at how this could be done.

lilleyse commented 6 months ago

EDIT: Maybe I misunderstood this. Maybe you meant to always pass the resourceDirectory to gltf-pipeline, regardless of whether it was given at the command line? That could be doable as well, with some care about input/output being 3DTILES/3TZ and such. I can have another look...

Yes, that's what I was suggesting

javagl commented 6 months ago

Yeah, this might be doable, but ... may be a bit awkward, and often not make sense. Literally none of the code works with "files". It obtains a Buffer from a TilesetSource (based on a URI string), does something, and writes it back to a TilesetTarget. Passing a "file path" along would not work when the input is a 3DTILES or 3TZ file.

(We might consider changing the resourceDirectory of gltf-piepline to a function (p: string) => Buffer as a similar generalization, but maybe that's out of scope).

So .... it might be technically possible, for the case of input files .... the question is then rather: Can it be implemented in a form that isn't toooo ugly.

Insopitus commented 6 months ago

A few first analysis steps:

1. The error message is indeed coming from gltf-pipeline. The B3DM files of the input contain to glTF 1.0 (GLB) files. The gltf-pipeline is used to update these to glTF 2.0 (involving some guesses and best-effort steps...). Now, the structure of the tileset here is a bit unusual: The GLB files refer to external textures (which are given as JPG files). Usually, these should be contained in the GLB itself.

However, this can be solved by passing in the resourceDirectory to the upgrade command. (The options are untyped and not documented extensively, but ... you know, JavaScript, you can throw in whatever you want). So it should be possible to perform the upgrade with a call like this: 3d-tiles-tools upgrade -i ./input/tileset.json -o ./output/tileset.json -f --options --resourceDirectory ./input/ (Note the --resourceDirectory ... part that basically points to the place where the JPG files can be found)

2.. But...

There seems to be something wrong with one of the input B3DMs. All of them seem to use a really old legacy version of B3DM. And that's ... "ok-ish". The upgrade should be capable of handling exactly that. But the Tile_+000_+000_+000.b3dm file seems to be "invalid/broken" in some way (even for the legacy format). I have not yet analyzed this in detail. It does not seem to be used in the tileset.json after all - maybe it can just be deleted? Otherwise, I'd have to dive into that, and see whether its contents could be salvaged...

3. The bounding volume is waaay off

Loading the original (legacy) tileset in CesiumJS and showing the bounding volume gives this:

Cesium tools issue 109 image 01

The fact that the content is not contained in the bounding volume will cause all sorts of rendering glitches, eventually. And ... this is not fixed via the upgrade process either.

There are some potential options for fixing the bounding volumes. But for now, this would only be a quick workaround for something that could/should be addressed more holistically via something like #106

Thank you so much for the details! I'll try it out on Monday when I have access to my work computer with other models.

We have very old and off-specification 3d tiles and people who write the generators didn't try to upgrade them. Sigh. I'll inform them about the bounding volume bug.

It's a relief the tool works on our model (I tested it on the test tiles I uploaded). I appreciate you guys' work.

javagl commented 6 months ago

If there are any problems with the other/full models (beyond the root B3DM file, which apparently is ~"broken"), then just drop a note here, preferably with further infos and maybe example data.

Even when the upgrade command cannot handle a certain unusual, obscure legacy input, there is a good chance that a few lines of custom code could make it work. And depending on what the input is, it might even make sense to handle this sort of input in the upgrade command in the future. For example, the B3DM with external JPG are something that should probably be handled (and I'll either leave this issue open, or track this in a new issue...).

Insopitus commented 5 months ago

@javagl I tried the workaround with a full 3d tiles model, and sadly, that didn't work. The --resourceDirectory approach seems only works when all the b3dm are directly in the "resource directory", not in any subdirectories.

a minimal test model

I have to pass --resourceDirectory ./Tile_+006_+003 to make the upgrade work, but there are other subtrees.

And depending on what the input is, it might even make sense to handle this sort of input in the upgrade command in the future. For example, the B3DM with external JPG are something that should probably be handled

I seems like this kind of problem can only be solved by the 3d-tiles-tools side to provide resource directories relative to the b3dm files to gltf-pipeline.

javagl commented 5 months ago

I see. When there are multiple B3DM files in different subdirectories, and each of them would require a different resourceDirectory, then passing a single, "global" resourceDirectory as an option to the gltf-pipeline won't be sufficient.

(I'm more and more strongly considering to just open a PR into gltf-pipeline to optionally pass in some resourceResolver: (p: string) => Promise<Buffer>, but there are some aspects that have to be sorted out for that...)

I'll have a close look at the example that you attached ASAP, and ...

javagl commented 5 months ago

I had a short look at the latest attachment. It still seems to have a structure that could be processed with the resourceDirectory parameter (because the paths to the shaders are stored as starting with ../Shaders). But I think that I understood the problem in general - namely, potentially having to define multiple resourceDirectory values, one for each B3DM file.

I therefore tried to create a test data set, based on the initial one, where I just moved two of the tiles into TileA and TileB subdirectories. I hope this matches the structure of the actual data. I'll attach it here for reference:

manual-input-2024-03-25.zip


Now, as I said, it should be possble to do the update manually, with a small workaround script. Based on the current state of the 3d-tiles-tools repo, it should be possible to put the following into the root directory of the project, and execute it with npx ts-node ManualUpgradeIssue109.ts

It essentially does what is usually done by the upgrade command under the hood. (Some parts are omitted. For example, it does not handle I3DM files. If the real input involves I3DM files, some aspects may have to be tweaked here).

The upgrade of the tileset.json is done as it was done originally, as part of the upgrade.

The upgrade of the B3DM files is "carved out", into a processEntry function (and I added a few comments there). This function will now determine the resourceDirectory that has to be used, for each B3DM, and pass this to the actual upgrade function as part of the options for gltf-pipeline.

(Note: Adjust the sourceDirectoryRoot and targetDirectoryRoot in the code according to your input/output directories, respectively)

import path from "path";

import { ContentDataTypes } from "./src/base";
import { Tileset } from "./src/structure";
import { Schema } from "./src/structure";
import { TilesetEntry } from "./src/tilesets";
import { BasicTilesetProcessor } from "./src/tools/tilesetProcessing/BasicTilesetProcessor";
import { TilesetObjectUpgrader } from "./src/tools/tilesetProcessing/upgrade/TilesetObjectUpgrader";
import { TilesetUpgradeOptions } from "./src/tools/tilesetProcessing/upgrade/TilesetUpgradeOptions";
import { ContentUpgrades } from "./src/tools/contentProcessing/ContentUpgrades";

const sourceDirectoryRoot = "D:/manual/";
const targetDirectoryRoot = "D:/manual-out/";

const sourceName = path.resolve(sourceDirectoryRoot, "tileset.json");
const targetName = path.resolve(targetDirectoryRoot, "tileset.json");

// Process a single entry of the input. This is called for all files
// of the input tileset that appear as content URI.
//
// The 'sourceEntry' a structure with these properties:
// - key: string   // The name of the content, like "/example/tile.b3dm"
// - value: Buffer // The content of that file, as a buffer
//
// The 'type' is one of the strings defined in 'ContentDataTypes'.
//
// The function returns a 'target entry', which contains
// - key: string   // The same as the source entry key
// - value: Buffer // The upgraded file contents
//
// For most file types, the target entry will be the same as the
// source entry. ONLY for B3DM files, the target entry value
// will be a buffer that contains the upgraded B3DM data.
//
const processEntry = async (
  sourceEntry: TilesetEntry,
  type: string | undefined
) => {
  // All files except for B3DM files remain unmodified
  if (type !== ContentDataTypes.CONTENT_TYPE_B3DM) {
    return sourceEntry;
  }

  // For B3DM files, determine the 'resourceDirectory' for
  // the gltf-pipeline, based on the source root directory
  // and the (relative) path that of the URI that points
  // to the B3DM
  const b3dmPath = path.resolve(sourceDirectoryRoot, sourceEntry.key);
  const b3dmDir = path.dirname(b3dmPath);
  const gltfUpgradeOptions = {
    resourceDirectory: b3dmDir,
  };
  console.log(
    "Upgrade " + sourceEntry.key + " with resource directory " + b3dmDir
  );
  const targetValue = await ContentUpgrades.upgradeB3dmGltf1ToGltf2(
    sourceEntry.value,
    gltfUpgradeOptions
  );
  return {
    key: sourceEntry.key,
    value: targetValue,
  };
};

async function run(tilesetSourceName: string, tilesetTargetName: string) {
  const overwrite = true;
  const processExternalTilesets = true;
  const tilesetProcessor = new BasicTilesetProcessor(processExternalTilesets);
  await tilesetProcessor.begin(tilesetSourceName, tilesetTargetName, overwrite);

  // The default options for the upgrade. (These mainly
  // refer to the tileset JSON upgrades)
  const upgradeOptions: TilesetUpgradeOptions = {
    upgradeExternalTilesets: true,

    upgradedAssetVersionNumber: "1.0",
    upgradeRefineCase: true,
    upgradeContentUrlToUri: true,
    upgradeEmptyChildrenToUndefined: true,

    upgradeContentGltfExtensionDeclarations: false,

    upgradeB3dmGltf1ToGltf2: true,
    upgradeI3dmGltf1ToGltf2: true,

    upgradePntsToGlb: false,
    upgradeB3dmToGlb: false,
    upgradeI3dmToGlb: false,
  };

  // Perform theu upgrade of the tileset JSON with the default options
  await tilesetProcessor.forTileset(
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    async (tileset: Tileset, schema: Schema | undefined) => {
      const tilesetObjectUpgrader = new TilesetObjectUpgrader(upgradeOptions);
      await tilesetObjectUpgrader.upgradeTilesetObject(tileset);
      return tileset;
    }
  );

  // Process all entries (files) in the input that are
  // considered to be "tile content". In "processEntry",
  // only B3DM files will be considered
  await tilesetProcessor.processTileContentEntries(
    (uri: string) => uri,
    processEntry
  );

  await tilesetProcessor.end();
}

run(sourceName, targetName);

When running this locally, based on the test data, then the output is something like

Upgrade TileA/Tile_+000_+000_+000_L16_0.b3dm with resource directory D:\manual\TileA
Upgrade TileB/Tile_+000_+000_+000_L17_00.b3dm with resource directory D:\manual\TileB

showing that it seems to pick up the right resourceDirectory for each of the B3DMs.

So if this works in principle, we can think about how to make this part of the standard upgrade command.

(Technically, this should be easy, but this snippet uses the global sourceDirectoryRoot variable to determine the full resourceDirectory, and ... we'll have to find a nice way to do this without this global variable...)

Insopitus commented 5 months ago

I run the script and an The "path" argument must be of type string. Received undefined error was thrown.

I set the sourceDirectoryRoot and targetDirectoryRoot variables back and uses the model you attached, and it works fine.

I'll have a deeper look at the error and send you a feedback tomorrow.

javagl commented 5 months ago

From the message, a first debugging step could be to either insert a log like

console.log("Resolving " + sourceEntry.key + " against " + sourceDirectoryRoot);
const b3dmPath = path.resolve(sourceDirectoryRoot, sourceEntry.key);

or run it in a debugger. (If you can share data where the error happens, I can have another look as well)

Insopitus commented 5 months ago

Sorry for the late reply. I was quite busy yesterday.

I found out the error occured because of const sourceContentUri = content.uri in line 426 of BasicTilesetProcessor.ts, the content object has a url field rather than uri. I replaced it with const sourceContentUri = content.uri || content.url and the upgrading succeeded.

Thanks a lot for the help.

javagl commented 5 months ago

That's a bit surprising: There is a flag for that, upgradeContentUrlToUri in the upgradeOptions. So upgrading the content.url to content.uri should actually be done by the TilesetObjectUpgrader. And this change should then already be picked up during the content upgrades. I'll try to reproduce this locally, maybe I can figure out why that happened.

javagl commented 5 months ago

As I said above: The cases where a content uses a (legacy) url property instead of a uri property should already be handled by the upgrade.

Two steps for analyzing that further could be:

One could add Loggers.get("upgrade").level = 'trace'; at the top of that workaround script, to set the log level for the "upgrade" part to trace. I tried this out with a tileset where I manually changed the uri to url, and it caused the expected messages to be printed:

DEBUG (upgrade): Upgrading content.url to content.uri
TRACE (upgrade):   Renaming 'url' property for content TileA/Tile_+000_+000_+000_L16_0.b3dm to 'uri' 
TRACE (upgrade):   Renaming 'url' property for content TileB/Tile_+000_+000_+000_L17_00.b3dm to 'uri'

(I have to check whether this might not work as expected in the case of external tilesets. These should just undergo the same upgrade process, but maybe there's a corner case that is not considered)

The other step (already mentioned above) would be to add the log statement at

console.log("Resolving " + sourceEntry.key + " against " + sourceDirectoryRoot);
const b3dmPath = path.resolve(sourceDirectoryRoot, sourceEntry.key);

and if this receives undefined, one could try to figure out where this undefined comes from (with a breakpoint/debugger run).