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(loader-utils): Example working on range requests #2966

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dariaterekhova-actionengine
Copy link
Collaborator

I added there main mechanism for loading slpk via range requests. Optimisation will be implemented in the future PRs so it works only on relatively small datasets for now.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

We now have a full set of FileProvider subclasses that duplicates the same thing as the ReadableFile Subclasses. When we are going to align :)

// loaders.gl
// SPDX-License-Identifier: MIT
// Copyright (c) vis.gl contributors
import {FileProvider} from './file-provider';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check differences with HTTPFile (which does inherit from ReadableFil):

headers: {Range: `bytes=${nOffset}-${nOffset + nLength - 1}`}

We should really have some tests for this. Maybe we can mock fetch?

* @param lenght Length of read data
*/
private async getBytesFromFile(start: number, lenght: number): Promise<ArrayBuffer> {
const res = await fetch(this.url, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My worry with FileProvider is that makes lots of tiny reads - that will absolutely kill performance for anything network based.

height: 100%;
}

body {
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 need so much styling. The smaller we make the example source files, the better.

"maplibre-gl": "^3.6.2",
"prop-types": "^15.7.2",
"react": "^18.2.0",
"react-color": "^2.19.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are all dependencies used? Again try to minimize.

import {Tile3DLayer, Tile3DLayerProps} from '@deck.gl/geo-layers';
import {UpdateParameters} from '@deck.gl/core/typed';

export default class CustomTile3DLayer<
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comments explaining what this custom layer does, why we needed to subclass it. The switch to range requests should ideally not require any changes to the layer?

@@ -21,39 +21,39 @@ export class BrowserFile implements FileProvider {
* @param lenght Length of read data
*/
private async getBytesFromFile(start: number, lenght: number): Promise<ArrayBuffer> {
let reader = new FileReader();
const reader = new FileReader();
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • We already have blob-file.ts that handles this.
  • AFAIK, there is no advantage to use FileReader over await Blob.arrayBuffer();

worker: false
};
// @ts-expect-error
const layers = new CustomTile3DLayer({
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
const layers = new CustomTile3DLayer({
const layer = new CustomTile3DLayer({

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants