-
Notifications
You must be signed in to change notification settings - Fork 284
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
Drag and Drop JSON File to Tile Layer to Reconstruct Saved Map #1345
Drag and Drop JSON File to Tile Layer to Reconstruct Saved Map #1345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really good. What do you think of integrating this into archive.js?
Oh! okay... sounds good too. I'll sure do. |
Hi @jywarren, drag and drop feature (for image and JSON file) now integrated into archive.js. Kindly review. Illustrations 2, 3 and 4 below are for your reference. |
This is looking good! Is local.js still necessary to change? Also, I wasn't sure I followed, is it recovering the corner latitude longitude pairs and placing images where they had been? |
Is local.js still necessary to change? No, the new functionality integrated into archive.js is completely independent of local.js Is it recovering the corner latitude longitude pairs and placing images where they had been? Yes. This means, archive.js does two additional things: 1. supports dragging and dropping of images to the tile 2. fetches, places images (on the tile) in coordinates specified in a JSON file that must be dragged and dropped on the tile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I was just trying to test out the code myself and it didn't quite work so I checked and had a couple clarifications. It looks super close, hopefully we can resolve these and we'll be there! Great work!
@@ -33,13 +33,25 @@ const uploadFiles = () => { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, i just still see some changes to this file so I asked, maybe this was unintentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's intentional, do you want me to remove the changes since focus has since shifted to archive.js?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of the changes, does it do the same thing for the local.html demo? That makes sense if so!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes expand existing drag and drop functionality in local.html/local.js to include support for JSON file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, great, let's keep it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, many thanks!
examples/js/archive.js
Outdated
const imgObj = JSON.parse(reader.result); | ||
// for json file with multiple image property sets | ||
if (imgObj.images.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, should we be reading .images
? When I look at an example JSON file that's been generated from MapKnitter lite, it doesn't have this property, it's just an array, like this:
[{"id":1504,"src":"https://archive.org/download/sanborn-1885/sanborn-1885.jpg","width":7573,"height":6396,"image_file_name":"sanborn-1885.jpg","nodes":[{"lat":39.32869582897733,"lon":-120.187548995018},{"lat":39.32982863334289,"lon":-120.18338084220888},{"lat":39.3270941035765,"lon":-120.182141661644},{"lat":39.32596125491416,"lon":-120.18630981445314}],"cm_per_pixel":10.875006010132818}]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! okay.. I will look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jywarren, so I tested the feature with segeotest2.json
and everything works fine, so no problem really. However, if you compare this file with test2.json
(generated from Mapknitter lite, that you used in your test), you will observe a difference. Some properties (e.g., avg_cm_per_pixel
, images
- this is an array) are missing in test2.json
but available in segeotest2.json
. That's why the test you did failed.
I think format in segeotest2.json
correctly matches (except for order and the key-value pair on tooltipText) the expected JSON output from Illustration 7 and that there's instead a need to rework Mapknitter Lite to produce JSON output similar to segeotest2.json
. What do you think?
See illustrations 5, 6 & 7 below.
ILLUSTRATION 5 - test2.json (generated from Mapknitter Lite)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for figuring this out! Where is your JSON file from? It looks like MK Lite follows the format shown here:
https://github.com/publiclab/image-sequencer-app
Of course we could accommodate both formats. But it does seem helpful to know where the two each come from and to standardize. One reason to use the version listed at the URL above is that there is a pregenerated saved JSON file for every single map in the old MapKnitter application, in the internet archive:
https://web.archive.org/web/20220726175911/https://mapknitter.org/maps/ceres--2/warpables.json
So in theory we could adapt this to display any legacy MapKnitter map, if we maintain the same exact format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, as for where the two formats came from:
- test2.json: if you click save icon on MK lite, you get a json file formatted into what obtains in test2.json.
- segeotest2.json: this comes from the json object returned from
generateExportJson()
in distortableCollection.js.
Note that the format shown in https://github.com/publiclab/image-sequencer-app is at variance with the json object returned by generateExportJson()
.
So should we support both formats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosh, this is a mess, sorry! I looked carefully and actually image-sequencer-app accepts an object with a collection
array, not an images
array; you can see it here:
form.append('collection', JSON.stringify(mergedOpts.collection)); |
and here:
https://github.com/publiclab/image-sequencer-app#usage
and unfortunately, we even convert from images
to collection
here:
Leaflet.DistortableImage/src/edit/DistortableCollection.Edit.js
Lines 327 to 330 in 8cca752
// If the user has passed collection property | |
this.customCollection = !!opts.collection; | |
if (!this.customCollection) { | |
opts.collection = this._group.generateExportJson().images; |
This must have happened as multiple people over the years wrote code and didn't manage to coordinate. It really should be standardized. I believe generateExportJson
is the only place images
appears; elsewhere, I believe we use collection
. Let's stick with that, what do you think:
{
"collection": [
{ "nodes": [ ... ], src: "" },
{ "nodes": [ ... ], src: "" }
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost tempted to try to fix this internal inconsistency, as it's annoying and confusing. But for now, maybe, let's just get the output and repackage it using collection
as in the DistortableCollection.Edit.js
code above. How does that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost tempted to try to fix this internal inconsistency, as it's annoying and confusing. But for now, maybe, let's just get the output and repackage it using
collection
as in theDistortableCollection.Edit.js
code above. How does that sound?
Since images
is only used in generateExportJson
, your suggestion sounds good.
However, on this strength and if benign, probably an alternative approach could be to refactor generateExportJson
to return an object formatted into collection
as presented in code snippet above. This way, we bite the bullet and make the story straight at once. In any case, other dependent source files (I believe recent files only) will also be refactored accordingly. What do you think?
Hi @jywarren, this is for your review. The code has been refactored to support drag and drop for local.html, archive.html as well as reconstructing images (from URL) using the agreed JSON format. Also, the save icon on sidebar now writes to disk JSON file consistent with the agreed JSON format. The below is for your reference. JSON files downloaded from MK Lite, they now use the new JSON format agreed on:
URLs used for testing URL-based image reconstruction:
Note: As for |
Exciting!! So, I tried making a map, downloading it, then refreshing the
page and dragging it back on after dismissing the welcome modal. But I got
an error! Can you try this workflow? I can record the errors too.
…On Tue, Feb 7, 2023, 6:58 PM Segun ***@***.***> wrote:
Hi @jywarren <https://github.com/jywarren>, this is for your review.
The code has been refactored to support drag and drop for local.html,
archive.html as well as reconstructing images (from URL) using the agreed
JSON format. Also, the save icon on sidebar now writes to disk JSON file
consistent with the agreed JSON format. The below is for your reference.
JSON files downloaded from MK Lite, they now use the new JSON format
agreed on:
1. https://ia804700.us.archive.org/5/items/mkl-1/mkl-1.json
2. https://ia804700.us.archive.org/5/items/mkl-3/mkl-3.json
URLs used for testing URL-based image reconstruction:
1.
http://localhost:8081/examples/archive.html?json=https://archive.org/download/mkl-1/mkl-1.json
2.
http://localhost:8081/examples/archive.html?json=https://archive.org/download/mkl-3/mkl-3.json
Note: As for files[0].type === 'application/json' on line 346 inside
archive.js, the property type seems to be introduced by the event object
passed to handleDrop().
—
Reply to this email directly, view it on GitHub
<#1345 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAF6J6URJFV55XQ2BJ6BDLWWLORPANCNFSM6AAAAAAUPSQWS4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Here's the error I got:
And here's the .json: {"avg_cm_per_pixel":6.763221163059949,
"collection":[{"id":134,"src":"https://archive.org/download/texas-barnraising//IMG_0137.JPG","width":3648,"height":2736,"tooltipText":"IMG_0137","image_file_name":"IMG_0137.JPG",
"nodes":[
{"lat":51.49046894696402,"lon":-0.06231307983398438},
{"lat":51.48565891027461,"lon":-0.03896713256835938},
{"lat":51.46427482966439,"lon":-0.03896713256835938},
{"lat":51.46427482966439,"lon":-0.08462905883789064}
],"cm_per_pixel":3.926851070243194},
{"id":132,"src":"https://archive.org/download/texas-barnraising//IMG_0128.JPG","width":3648,"height":2736,"tooltipText":"IMG_0128","image_file_name":"IMG_0128.JPG",
"nodes":[
{"lat":51.53629915545823,"lon":-0.15552520751953128},
{"lat":51.53629915545823,"lon":-0.10986328125000001},
{"lat":51.5027589576403,"lon":-0.12445449829101564},
{"lat":51.51493882813492,"lon":-0.15552520751953128}
],"cm_per_pixel":7.291666666666667},
{"id":120,"src":"https://archive.org/download/texas-barnraising//IMG_0114.JPG","width":3648,"height":2736,"tooltipText":"IMG_0114","image_file_name":"IMG_0114.JPG",
"nodes":[
{"lat":51.499766912405946,"lon":-0.1694297790527344},
{"lat":51.486834743880806,"lon":-0.11655807495117189},
{"lat":51.47838944760882,"lon":-0.12376785278320314},
{"lat":51.47838944760882,"lon":-0.1694297790527344}
],"cm_per_pixel":9.071145752269986}]} The JSON looks OK, but I think somehow it's not correctly reading the coordinates? |
Somehow, every effort to reproduce the error failed as the browser console didn't return any error message. Everything works each time I followed the flow you suggested. For instance, see illustration 8 below. However, I observed a different issue which I will present separately in the following exchanges. |
Wow, that is strange. Can you try manually creating the image from the JS console, using: var image = L.distortableImageOverlay(
imageURL,
{
corners: ARRAY
}
).addTo(map); Maybe we can check if the locations recorded by the JSON exporter are themselves correct, apart from the reconstruction? |
You could also try to access the overlay image and run |
getCorners() for both before exporting, and after importing, to check the corner locations at various stages? |
@7malikk how does this process work for you, can you follow the steps and confirm what we're seeing? Thank you! |
I just realized also - the cached JSON files from legacy MapKnitter use the plain https://web.archive.org/web/20220726175911/https://mapknitter.org/maps/ceres--2/warpables.json This doesn't have to be a problem for us, certainly not now. The format we've chosen is definitely better. I'll make an issue to have a feature which can detect if the JSON is just an array, and accept that format too, for backwards compatibility. We can work on it later! |
Sounds good |
Oh! interesting, okay. I can work on this adapter in another PR once work on this PR is completed. |
Sure, I will give this and the previous two suggestions a shot. Many thanks! |
Hi @jywarren, based on Illustration 8 above, you can see everything works fine on my end following the flow you suggested. Do you want to re-attempt the process again and see if you still get that same error. If no error, I'd suggest you consider this PR for merging while I open another issue for the repositioning of reconstructed images in right coordinates. What do you say? |
I got the error below while trying to use JS console. So for this reason, I haven't been able to run the tests exactly the way you advised (as above and in the two suggestions you gave). However, I replicated a similar test from within the MK lite code but didn't find any inconsistency in the coordinates. I'm thinking we spin-off the issue of proper repositioning of reconstructed images into a stand-alone issue and tackle it therefrom. What do you think? |
OK! Locations look good to me. I saw this error: but images were properly placed after interacting with the map and panning to where the images should be. One challenge was getting back to the same place, so i added this after images are reconstructed:
I noticed you were setting image height, and followed where you traced the code to see that was used. But then I realized, the code relies on the image being loaded to measure the height and width, and if we're not at the correct location, the images are off-screen, and they never get loaded (i think Leaflet is smart like that, only loading them if they're onscreen). So that's why we didn't get the width and height, and that's why the error happens! To test this idea, I tried manually panning to the correct location before dragging the JSON file onto the map. And indeed, they loaded correctly with no error then! OK, so that means, we have to pan to the correct location before we can load the images. We actually need to parse through the image corners and collect an array of all the lat/lon corners, and create a https://leafletjs.com/reference.html#latlngbounds Then we should be able to run:
BEFORE we place the images, and then they should generate properly. OK, I left a few comments and changes in the code to illustrate this, and where to put the bounds collecting code! |
image = L.distortableImageOverlay( | ||
imageURL, | ||
{ | ||
height: options.height, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed height here because it'd be auto-detected if we do the fitBounds thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... okay.
examples/js/local.js
Outdated
} else { | ||
// reconstructs map into previous state | ||
image = L.distortableImageOverlay(imageUrl, { | ||
height: options.height, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to make the same changes to local.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure... okay.
examples/js/archive.js
Outdated
@@ -249,6 +251,9 @@ document.addEventListener('DOMContentLoaded', async (event) => { | |||
let imageURL; | |||
let options; | |||
|
|||
// let cornerBounds = .... // collect all corners into a big array of [lat, lon] | |||
// map.fitBounds(cornerBounds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's where we can collect up all the corner lat/lon pairs, and make an array. Then on the second line, we fit the window to contain them all before we start placing images!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay... many thanks!
Okay, this is a very helpful finding. I applied it and it worked, thank you!!! By the way, I noticed that specifying the height is not a problem but well, I left it out as per your advice. |
Super. Good to merge now? Great job! |
Reconstructed images (by Dnd or JSON URL) are placed on the right coordinates now, thanks to Tests were performed using the MK lite-generated JSON files (mkl-01.json & mkl-02.json) and JSON URLs below. |
Excellent! Merging!! |
Great work @segun-codes |
Fixes #1344
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
grunt test
Hi @jywarren, the feature to drag and drop .json files to tile layer for the reconstruction of map to its previous saved state is now up and running. Please, review and advise with your thoughts. See Illustration 1 below for some insights. Many thanks!
ILLUSTRATION 1
If tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!