DDMAL / pixel_wrapper

Rodan Pixel.js Wrapper
Other
0 stars 1 forks source link

Background layer zoom is wrong when exporting to image data/csv #19

Closed EricHanLiu closed 6 years ago

EricHanLiu commented 6 years ago

Works with highlights, but the issue is that exporting to image data or CSV causes Pixel to try to fetch the image data of each layer, which is what the background layer also does, but uses the currentZoom instead of maxZoom.

This causes a conflict somewhere which causes the background layer to be zoomed in a level higher than it should when being exported.

With the way pixel currently is coded, exporting to imageData/CSV bases the resolution on the current zoom instead of max zoom. This means that if I export while I'm viewing the image at lowest zoom, the exported layers will be lowest resolution.

The way we want the background layer is to always fetch based on the highest zoom so that the image is at the highest resolution (accurate # of pixels) for the Calvo classifier. Otherwise, if we base on current zoom similar to imageData/CSV, the background layer is not pixel-accurate (8 pixels per 1 actual image pixel) and doesn't display the whole layer.

Perhaps this needs to be changed in Pixel.js, so the exports are always done at the highest resolution?

EricHanLiu commented 6 years ago

To clarify, the exportToImageData/CSV work properly for the other layers. These features shouldn't be used in an actual Rodan workflow, since the user should only ever exportToRodan or exportAsHighlights (to see the layers themselves).

It's only the backgroundLayer that has the wrong zoom since we force maxZoom for the highest resolution since that is what's needed by CM.

If we do eventually manage to separate pixel from pixel_wrapper, and if we keep the backgroundLayer as a pixel_wrapper specific feature, then this wouldn't affect the original Pixel.js at all.

EricHanLiu commented 6 years ago

This isn't the case with highlights (since exportToHighlights doesn't fetch pixel data, it just converts the layers to blobs as is), so I could just remove the backgroundLayer functionality from exportToImageData and exportToCSV since it shouldn't be needed for those exports (not used in the standalone Pixel.js, only for the wrapper with Rodan).

If we want those exports to include the background layer then I believe it would require not using maxZoom for the backgroundLayer generation which defeats the purpose of using it with CM (unless we come up with another way to generate the background layer pixel-wise).

EricHanLiu commented 6 years ago

Seems like we have two options:

  1. Have the background layer generated at maxZoom (highest resolution, necessary for calvo classifier) and just remove the backgroundLayer generation from exportToImageData or exportToCSV (so only the original layers are exported)
  2. Change the background layer generation to thisZoom (could range from 0-5, depends on the current page zoom), fixes the imageData/CSV exports but causes the highlights to be exported at the same resolution as the page zoom, so it can be like half the actual image size (200x160 inside of a 400x320 image).

I think the best bet is honestly to just ditch the backgroundLayer in exportToImageData/CSV so those two exports just generate the original layers, since the purpose is to be compatible with CM. However, I'll have to edit the actual Pixel code, which would make isolating the pixel_wrapper elements in the future a bit harder.

Any advice @napulen ?

EricHanLiu commented 6 years ago

Leaving a branch with the backgroundLayer ignored for the exportToImageData/CSV options, so backgroundLayer functionality only works with exportToHighlightsPNG or exportToRodan. Not sure what we want to do from here!

calvozaragoza commented 6 years ago

@EricHanLiu I think we don't really need to export the background layer. I can modify CM code to infer the actual background layer according to those pixels that do not belong to any other layer.

EricHanLiu commented 6 years ago

@calvozaragoza

Haha that's good to know, but the background layer has already been implemented and works properly. Since CM only requires the highlights, this issue shouldn't affect that at all. So as of now the background layer is generated and sent to CM properly.

Hopefully this saves you the work of having to change the CM code.

If it's a much faster/more elegant fix to do it on your end then let me know, but as of now it's already implemented in Pixel!

napulen commented 6 years ago

Sorry for getting into this just now.

Before we move on in-depth with the solution for the zoom problem, just a dumb question, @EricHanLiu are we completely certain that the export problem with the background layer is due to the zoom level? What is the evidence for that? I remember I still had doubts.

Given that said, I am more inclined towards your first proposal, we might not need the background layer in exportToImageData/CSV.

EricHanLiu commented 6 years ago

Hi @napulen , yes the issue is with the differing zooms. Because we force the background layer to be generated at highest resolution, it has the width and height of the original image. However, when exporting to data/csv the width and height of the canvas that's created depends on the current zoom, so if the user isn't maxZoomed then the width/height are smaller. Then I believe the pixel data that's drawn is only that within the smaller width/height.

For example, I have a 400x375 pixel image (panda), so the background layer is 400x375. However if I export while zoomed out then the backgroundLayer draws on only 200x188 (let's say) of the actual layer, ie. the top left corner.

To be honest, I'm not 100% sure of the exact intricacies of this zoom issue because the exportToImageData backgroundLayer sometimes does behave weirdly if I change my zoom (not as expected). But I do agree that it's not needed for those export types, so the solution could just be to ignore them.

This is the pull request

napulen commented 6 years ago

Alright, I am checking the request.

EricHanLiu commented 6 years ago

new pull request with squashed (and fixed) commits for clarity.

EricHanLiu commented 6 years ago

pull request accepted, closing