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

chore(tile-converter): Support for SLPKs larger than 2 Gb #2547

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

dariaterekhova-actionengine
Copy link
Collaborator

No description provided.

@dsavinov-actionengine dsavinov-actionengine changed the title chore(tile-converter): Support of SLPKs larger then 2 Gb chore(tile-converter): Support for SLPKs larger than 2 Gb Jul 13, 2023
@@ -12,6 +13,13 @@ export type SLPKLoaderOptions = LoaderOptions & {
};
};

async function parseSLPK(data: ArrayBuffer, options: SLPKLoaderOptions = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add TSDoc comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Move the function under the loader declaration. It is general practice in loaders.gl that the loader is declared first

@@ -0,0 +1,26 @@
import {FileProvider} from 'modules/i3s/src/lib/parsers/parse-zip/file-provider';

export const searchFromTheEnd = async (file: FileProvider, target: number[]): Promise<bigint> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add TSDoc comments and tests

@@ -0,0 +1,70 @@
import {FileProvider} from './file-provider';

const toNumber = (bigint: bigint) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add TSDoc comments

/**
* Provides file data using DataView
*/
export class DataViewFileProvider implements FileProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add tests for the class

}

/**
* Gets an unsigned 16-bit integer (unsigned byte) at the specified byte offset from the start of the file.
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsigned 16-bit integer != unsigned byte. It is unsigned short

Comment on lines 27 to 29
getUint8(offset: bigint): Promise<number> {
return Promise.resolve(this.file.getUint8(toNumber(offset)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
getUint8(offset: bigint): Promise<number> {
return Promise.resolve(this.file.getUint8(toNumber(offset)));
}
async getUint8(offset: bigint): Promise<number> {
return this.file.getUint8(toNumber(offset));
}

and for all async methods...


let offsetInZip64Data = 4n;
// looking for info that might be also be in zip64 extra field
if (uncompressedSize === BigInt(0xffffffff)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add test cases for these cases?

Comment on lines 75 to 76
* Gets an unsigned 32-bit integer (unsigned byte) at the specified byte offset from the start of the file.
* @param offset The offset, in bytes, from the start of the file where to read the data.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment looks like copy&paste from getUint32

buffer: Buffer;
};

export class FileHandle {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add TSDoc comments and tests

Comment on lines 27 to 29
export default class SLPKConverter {
/**
* extract slpk to i3s
* Convert slpk to i3s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename

@@ -6,17 +6,17 @@ import {FileProvider} from './file-provider';
*/
export type ZipCDFileHeader = {
/** Compressed size */
compressedSize: number;
compressedSize: bigint;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need bigints
image


const uncompressedSize = await buffer.getUint32(
headerOffset + offsets.CD_UNCOMPRESSED_SIZE_OFFSET
let uncompressedSize = BigInt(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please test BigInts on Safari / iOS. It should be supported now but better find out early

@@ -0,0 +1,34 @@
import test from 'tape-promise/tape';
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI: It is possible to make *.ts tests

@@ -0,0 +1,2 @@
/** Description of zip signature type */
export type ZipSignature = [number, number, number, number];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit. The type is so simple. The better is just declare a variable with this tuple:

const zipSignature: [number, number, number, number] = [1,2,3,4];

The separate file is too much for it.

@dariaterekhova-actionengine dariaterekhova-actionengine deleted the dt/serve-big-slpk branch July 20, 2023 13:58
@dsavinov-actionengine dsavinov-actionengine added the ActionEngine The issue is on ActionEngine controll label Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ActionEngine The issue is on ActionEngine controll
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants