-
Notifications
You must be signed in to change notification settings - Fork 5
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
Incorrect overlay Problem #23
Comments
cc @jywarren |
Can you show with a screenshot what it's supposed to look like? |
Thanks! |
Hey, it looks like perhaps you're just overlaying them in reverse order? |
I'm sorry, how's that? I mean the order of overlay won't change their relative positions right? |
Ah, i'm sorry, i didn't look close enough. Here they are overlaid: It looks like there may be a couple possible issues, because it doesn't look like you've simply got the x-axis scale wrong, it seems the rightmost image is actually taller than it should be as well. But we should remember that on MapKnitter it's shown in spherical mercator, whereas you're working in unprojected lat/lon, so maybe that part is OK. Most importantly it looks like the overlaps aren't right. Let me look closer... |
Hmm. the Ruby code relies on |
Sorry, what's going on here? Maybe we should add a comment? image-sequencer-app/src/api/v2/util/converter-multiSequencer.js Lines 25 to 28 in 560be72
|
Oh, I added that temporarily, it brings the final canvas size in range of the max allowed array size. We should be able to remove this once we use scale to determine the final size. |
Hmm, actually, how is
|
Oh, OK! So the width field in the json represents the actual image width then? |
Can you write a simple test for Actually I don't think you need to be using the |
Hmm. OK, so https://github.com/publiclab/image-sequencer/blob/main/src/modules/WebglDistort/info.json can already determine the height and width of the image, so it doesn't need |
Oh okay! This makes sense then! |
Yeah it just needs the four corner points. |
Do you think If we do not scale at all, it will work?? |
So maybe this should be: coords.push({
x: Math.round((node.lon - minX) / (maxX - minX)),
y: Math.round((node.lat - minY) / (maxY - minY))
}); But there's another problem - and this might be our issue -- we can't round lat/lon because even 0.000 is a matter of meters! |
Oh wait!! Another issue!! I just checked the json and the lat, lon values are strings! They need to be parsed first! |
Actually this is why I decided to scale it in the first place! |
For now, let's not scale. Once we convert to true projected
Here actually |
Okay so pxperm is consumed via the query parameter then, correct? |
the passed So if we must convert to pixels here, let's use a projection: |
instead of an arbitrary scale factor. |
Okay, this makes sense! So we ditch the current way of calculating height and width and then using px_per_m we can compute the final height and width of the image, correct? |
well, the values we pass into the webgl-distort module can be used to calculate the final height and width, so yeah, the values passed in originally are actually pre-distortion values. Maybe that's one thing that was broken! |
Okay, So I am finally starting to understand this! Yay! |
One more thing, what do we do if according to the given scale the size of the final canvas is too big for javascript array? Should we just reject the request then? Or modify the scale in some way? |
AWESOME!
…On Thu, Jul 11, 2019 at 2:21 PM Varun Gupta ***@***.***> wrote:
Okay, So I am starting to understand this! Yay!
Let me make some changes like you suggested and add a test like you asked
and we can pick this up from there! 😃
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23?email_source=notifications&email_token=AAAF6JZEKPLK7RWJDNMXJHLP652X3A5CNFSM4H7LZG3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXR2FY#issuecomment-510598423>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J5DQGH7O7LI65F3KVTP652X3ANCNFSM4H7LZG3A>
.
|
Made the change in #24! |
awesome!
…On Thu, Aug 22, 2019 at 12:33 PM Varun Gupta ***@***.***> wrote:
@jywarren <https://github.com/jywarren> Moving over from #24
<#24>
Working on a module which would resize the image to the desired width and
height. After that we should be able to debug the overlay issue more
clearly!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23?email_source=notifications&email_token=AAAF6J7KBMRLS3AYVKPDWNLQF25SHA5CNFSM4H7LZG3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45U42A#issuecomment-523980392>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J62I2K6XWM7NZXBZ53QF25SHANCNFSM4H7LZG3A>
.
|
Okay I think this should work https://www.npmjs.com/package/image-sequencer-app-resize |
Oh, whoa!!! It looks very close, Varun! Great work, i'm sure we're almost
there!!!
…On Sat, Aug 24, 2019 at 11:42 AM Varun Gupta ***@***.***> wrote:
Hmm, still not quite right! Let me make sure it does resize correctly.
[image: Screen Shot 2019-08-24 at 9 03 21 PM]
<https://user-images.githubusercontent.com/25617855/63639668-d1a1d380-c6b3-11e9-8267-558da0b70717.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23?email_source=notifications&email_token=AAAF6J7DDWJSZS32HD2PW4LQGFJFLA5CNFSM4H7LZG3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5CCQJY#issuecomment-524560423>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J3RF5IA3HPRUYNAX7TQGFJFLANCNFSM4H7LZG3A>
.
|
Hi @jywarren |
Ah, i see! The positions do look good and with the width fix, I'm really
optimistic about this! Thanks, Varun!
…On Fri, Sep 13, 2019 at 10:53 AM Varun Gupta ***@***.***> wrote:
Hi @jywarren <https://github.com/jywarren>
I think I got the problems with overlay resolved. This is the result
currently!
[image: Screen Shot 2019-09-13 at 8 18 50 PM]
<https://user-images.githubusercontent.com/25617855/64871943-e4c91300-d663-11e9-888a-17c6a0d5085b.png>
It does not line up because the resize module which I adapted from the
native resize module in image-sequencer isn't working properly on the width
so the images are coming out to be narrower than they should be. I'll write
a module for resizing from scratch using something like bilinear
interpolation now!
Sorry this took so long, I got caught up in some other stuff 😅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23?email_source=notifications&email_token=AAAF6J7DWPWFKHSYVIJD3QTQJOSLBA5CNFSM4H7LZG3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6VIKWQ#issuecomment-531268954>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAF6J3DPPIZSIAS2LNTKWDQJOSLBANCNFSM4H7LZG3A>
.
|
Yeah! I tried this out with a bunch of different scale values and it gives the same result for all of them! So I am sure that all we need now is to fix the resize! yay! |
Okay, a lot of debugging later, I finally figured this out! So what is happening is that after the distortion the image has some extra space which is not cropped out for some reason! Due to this the final size of the usable area of the image is coming out to be incorrect! @jywarren Any ideas how we can fix this?? Or is this expected behavior out of the webgl-distort module?? |
The inner rectangle is the whole area of the image but that has some extra space on the right side which was causing the image to come out narrower than we were expecting! |
One solution that comes to my mind is a |
We might wanna make changes to the wegl-distort module in the long term though! |
Wow, Varun, this looks perfect. Amazing work. So is this the initiating URL? I just opened this and it said http://34.74.118.242/api/v2/export/?url=https://mapknitter.org/maps/ceres--2/warpables.json |
Link to #28 |
@jywarren I think that's because we are missing the scale, try this: |
Or is it this? http://34.74.118.242/api/v2/export/?url=https://mapknitter.org/maps/ceres--2/warpables.json&upload=false&scale=50 The previous one had finally redirected to the full longer URL, but then also showed an error. Let's get the README updated so we know how to initiate this? Thanks, Varun! Again, fantastic work! |
Let's add that to the README, thanks! But also why am I getting intermittent |
Actually 50 I think would be too big for it to handle! We could start with something like 5. |
oh ok. maybe the scale is implemented differently that in mapknitter-exporter. 50 is a reasonable value there. But we can set up a conversion function and disambiguate later. Great, i'll try! |
@jywarren I think the warpable needs to use http too! |
Yeah, I think we gotta scale the scale lol. |
OK perfect, that worked! Can you make a quick edit to the README and actually add this test URL that people can use? Let's close this and open a new one for scale. |
The issue:
The distorted images are not being overlaid correctly with respect to each other on the final canvas.
The logic to calculate the distortion and overlay coordinates happens inside the converter util.
https://github.com/publiclab/image-sequencer-app/blob/dev/src/api/v2/util/converter-multiSequencer.js
Steps to reproduce the issue locally:
dev
branch.npm i
npm run start
The images are getting distorted correctly, to test that we can use this url:
http://localhost:4000/api/v2/export/?url=http://mapknitter.org/maps/pvdtest/warpables.json
Since the upload query param is not set, this should return the correctly distorted image as a response to the browser. I have added Logs on each step so it should be easy to see the status of the request.
Now, moving on to multi-image maps, we can use the ceres map to see the relative positioning of the images.
Using this endpoint
http://localhost:4000/api/v2/export/?url=https://mapknitter.org/maps/ceres--2/warpables.json
This should return the 3 images distorted and overlaid on a canvas but they won't quite match up.
Please Do not run this with larger image maps since the browser will make repeated requests to the server and you will not see any response.
The text was updated successfully, but these errors were encountered: