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

[GSDESCARTE-7] Handle errors in layer loading process #9

Closed
wants to merge 7 commits into from

Conversation

tkohr
Copy link
Member

@tkohr tkohr commented Dec 2, 2020

PR ascends (tile/image loading errors) into job status (by layer and tile) and allows to activate a debug mode per layer to render erroneous tiles pink.

Can be tested with erroneous layers, e.g. copying test data into demo directory and using larger extent:

{
  "layers": [
    {
      "type": "XYZ",
      "url": "/data/osm-tiles/{z}/{x}/{y}.png"
    }
  ],
  "size": [1024, 1024],
  "center": [12, 48],
  "dpi": 50,
  "scale": 40000000,
  "projection": "EPSG:3857"
}

TODOs:

  • show missing tiles with a specific color in the final image => can be configured via debug param in layer spec
  • still give an image to the user even if one or more layers fail => has already been the case
  • provide the user with the necessary information regarding errors => should raise an error state for a tile / layer in the job
  • deblock non-tiled WMS for debug: false
  • add and adapt tests (observable completion)

@tkohr tkohr force-pushed the GSDESCARTE-7-error-handling branch from 56dddd3 to 54c2f29 Compare December 4, 2020 15:25
@tkohr tkohr marked this pull request as ready for review December 14, 2020 08:16
Copy link
Member

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Good approach but i think it's too complex for nothing, errors don't have to be an observable.

if (allReady) {
for (let i = 0; i < layerStates.length; i++) {
context.drawImage(layerStates[i][1], 0, 0);
if (layerStates[i][2] && layerStates[i][2].length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

use explicit variable

const canvasImage = layerStates[i][1]
const errors = layerStates[i][2]

@@ -45,33 +45,42 @@ export function createJob(spec) {
.pipe(
switchMap((layerStates) => {
const allReady = layerStates.every(([progress]) => progress === 1);

let errors = [];
Copy link
Member

Choose a reason for hiding this comment

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

Should be typed

return {
...job,
progress,
imageBlob,
status: progress === 1 ? 'finished' : 'ongoing',
errors: errors,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
errors: errors,
errors,

const width = rootFrameState.size[0];
const height = rootFrameState.size[1];
const context = createCanvasContext2D(width, height);
context.canvas.style = {};
let frameState;
let layer;
let renderer;
let errors = [];
Copy link
Member

Choose a reason for hiding this comment

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

type


const update$ = interval(500);
let errorTile$ = new BehaviorSubject();
let errorWMS$ = new BehaviorSubject();
Copy link
Member

Choose a reason for hiding this comment

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

no need for observables here

xhr.addEventListener('loadend', function () {
var data = this.response;

if (this.status === 0 || this.status >= 400) {
Copy link
Member

Choose a reason for hiding this comment

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

set TileState.error here

if (debug) {
createErrorTile(image, tileSize, this.status);
}
errorTile$.next({ origin: tile.key, tile: tile, error: this.status });
Copy link
Member

Choose a reason for hiding this comment

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

not sure we need the size or the tile ..

if (debug) {
createErrorTile(image, tileSize, this.status);
}
errorTile$.next({ origin: tile.key, tile: tile, error: this.status });
Copy link
Member

Choose a reason for hiding this comment

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

origin should be the tile url, not the key

Comment on lines -103 to +148
return update$.pipe(
startWith(true),
let updateWhile$ = update$.pipe(
startWith(0),
takeWhile(() => {
frameState.tileQueue.reprioritize();
frameState.tileQueue.loadMoreTiles(12, 4);
return frameState.tileQueue.getTilesLoading();
}, true),
map(() => {
}, true)
);

return combineLatest(updateWhile$, errorTile$).pipe(
map((status) => {
let error = status[1];
// abort tile rendering if no pink error tiles are drawn
if (error && !debug) {
error.tile.setState(TileState.ERROR);
}
// check if error is not already present from previous interval
if (errors[errors.length - 1] !== error) {
// check if error comes from same source
if (source.key_ === error.tile.key) {
errors.push(error);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

remove

}
}),
throttleTime(500, undefined, { leading: true, trailing: true })
);
}

function createErrorTile(image, tileSize, errorCode) {
Copy link
Member

Choose a reason for hiding this comment

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

move to printer/utils

Copy link
Member

Choose a reason for hiding this comment

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

type definition

@tkohr
Copy link
Member Author

tkohr commented Dec 16, 2020

@fgravin thanks a lot for your feedback. I have opened a much simpler PR regarding this issue here #16

@fgravin
Copy link
Member

fgravin commented Dec 23, 2020

continued in #16

@fgravin fgravin closed this Dec 23, 2020
@fgravin fgravin deleted the GSDESCARTE-7-error-handling branch December 23, 2020 11:08
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