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

feat(cli): Create-overview determine max zoom overview from the gsd #2612

Merged
merged 4 commits into from
Nov 30, 2022

Conversation

Wentao-Kuang
Copy link
Contributor

No description provided.

@Wentao-Kuang Wentao-Kuang requested a review from a team as a code owner November 30, 2022 02:00
logger.info({ source, duration: st.tick() }, 'CreateOverview:ListTiffs:Done');

logger.debug({ source }, 'CreateOverview:PrepareSourceFiles');
const tileMatrix = await this.getTileMatrix(tiffSource);
const tiff = await CogTiff.create(tiffSource[0]);
Copy link
Member

Choose a reason for hiding this comment

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

this will throw if there are not tiffs, maybe check and exit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a filterTiff for the file lists. So I am ok if the script just die here if the not tiff?

Copy link
Member

Choose a reason for hiding this comment

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

we should let it die here, but with a nice error message, stack traces are not super helpful to users

@@ -18,6 +18,7 @@ import { JobTiles, tile } from './tile.generator.js';
import { SimpleTimer } from './timer.js';

const DefaultMaxZoom = 15;
const MaxNumberTiles = 250000;
Copy link
Member

Choose a reason for hiding this comment

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

could we add a sentence explaining what these numbers do?

@@ -115,6 +114,7 @@ export class CommandCreateOverview extends CommandLineAction {
qk = QuadKey.parent(qk);
}
}
if (tiles.size > MaxNumberTiles) this.prepareTiles(files, maxZoom - 1);
Copy link
Member

Choose a reason for hiding this comment

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

return?

Copy link
Member

Choose a reason for hiding this comment

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

also why is this async?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good pick

@@ -126,8 +126,7 @@ export class CommandCreateOverview extends CommandLineAction {
}
}

async getTileMatrix(sources: ChunkSource[]): Promise<TileMatrixSet> {
const tiff = await CogTiff.create(sources[0]);
async getTileMatrix(tiff: CogTiff): Promise<TileMatrixSet> {
Copy link
Member

Choose a reason for hiding this comment

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

this doesnt need to be async the tiff is tiff.init(true) before.

(you are initing the tiff twice)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need this to run await tiff.getImage(0).loadGeoTiffTags();, before getting the espg. The just tiff.init now working.
image

Copy link
Member

Choose a reason for hiding this comment

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

The tiff.init(true) is called in the other function you should pull that out to the same place as the new CogTiff

@kodiakhq kodiakhq bot merged commit 59a01e6 into master Nov 30, 2022
@kodiakhq kodiakhq bot deleted the feat/overview-on-gsd branch November 30, 2022 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants