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

Automated image matching and stitching using matcher-core library #312

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jywarren
Copy link
Member

by @rexagod

For discussion on integration with https://github.com/publiclab/matcher-core/issues/1 !!!

Copy link
Member Author

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

Hi! This is really impressive! Congrats!

Just noting that it's really helpful to open a PR as early as possible so that there can be feedback and discussion about the code as it evolves. I know it's helpful to be sort of secluded during tough coding sessions but as soon as possible please do post so we can discuss!

You've done some great work here! I left lots of feedback, and some questions/suggestions about code organization and structure. It'd also be great to see some docs in this PR that explain how to enable this feature in a map - not everyone will want to, so showing how it's supposed to be done helps people make a choice.

Thanks, @rexagod !!!

<script src="../node_modules/leaflet-toolbar/dist/leaflet.toolbar.js"></script>
<link href="../node_modules/leaflet-toolbar/dist/leaflet.toolbar.css" rel="stylesheet" />
<!-- for pattern-mining utility -->
<script src="../node_modules/matcher-core/orb.core.com.js"></script>
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we ought to move this to a unique example file called "matcher"?

Copy link
Member Author

Choose a reason for hiding this comment

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

@rexagod i think we'd better follow up on this, matcher.html sounds like a good name for the example!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, on it! 👍

<body>
<center>
<div class="loader-container">
<img width="100" src="leaflet.gif">
Copy link
Member Author

Choose a reason for hiding this comment

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

So, what is happening while this is showing? The matcher is searching for interest points in some images?

Copy link
Member

@rexagod rexagod Jul 17, 2019

Choose a reason for hiding this comment

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

Yup! But now with the caching implemented, I guess we won't be needing this at all from the second iteration onwards, and the initial iteration should take way lesser loading time (~7000 to ~15 frames perf improvement), so I guess it would be better if we should synchronously process those, and remove this loading screen altogether?

processedPoints.images[1].getCorner(3).lng - lng_offset
];
var zoom_level = map.getZoom();
map.setView(overlay.getCenter(), (zoom_level%2?zoom_level+1:zoom_level-1));
Copy link
Member Author

Choose a reason for hiding this comment

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

I was a bit disoriented when the map itself moved. Maybe it's not necessary?

Copy link
Member

Choose a reason for hiding this comment

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

This actually is something weird. What's happening is that after updating the corners here the changes are actually reflected into the UI after I change the view, maybe there's some sort of cache refresh when map zooms out/in (since the map changes)? This would explain why it's happening. However, I can't yet find a documented way to trigger the same explicity, without the need of changing the view. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

@rexagod haven't checked this for myself but I think you just need to call overlay._reset()?

Copy link
Member

Choose a reason for hiding this comment

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

actually via #380 setCorners / setCorner methods I exposed should do the trick

}
}
}
var best_point =
Copy link
Member Author

Choose a reason for hiding this comment

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

Could some of this be refactored into a subfunction called something like filterBestPoints() or something? That wouldn't have to mix at all with HTML as this file does?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Do you want me to expose an API here, or should this change be incorporated implicitly?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, should I extract this fn into a different file too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well if the ability to filter points gets moved back into the core with projector and doesn't mix with the DOM, does that make sense? It could be an exposed API so that the point selection metric could be overridden...

var corresponding_best_point =
processedPoints.points[1][processedPoints.confidences[0].indexOf(max_)];
document.querySelector(
"#map > div.leaflet-pane.leaflet-map-pane > div.leaflet-pane.leaflet-marker-pane"
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, we should probably not use #map here - different leaflet maps may have different IDs, and also, what if there is more than one Leaflet map per page? We should base this off of the Leaflet map object container!

A = B;
idx--;
}
window.processedPoints = processedPoints;
Copy link
Member Author

Choose a reason for hiding this comment

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

Could we be passing this around by reference instead of globally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whose processedPoints are these?

sectors.s01[i] = [];
sectors.s10[i] = [];
sectors.s11[i] = [];
center = processedPoints.images[i].getCenter();
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so shouldn't processedPoints be a property of each image, rather than the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, i get it. images[0] and images[1] are the two origin images for the match? And is processedPoints actually better named processedPairs? Or matchedPointPairs?

Copy link
Member Author

@jywarren jywarren left a comment

Choose a reason for hiding this comment

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

And if you could reply to the other comments here too that'd be great! Thanks!!!

@@ -0,0 +1,86 @@
function projector(utils, e, array, obj) { // jshint ignore:line
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @rexagod could this be abstracted to accept and produce coordinates independently from the Dom? It might be a good thing to add abstractly to the matcher-core lib?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so just to clarify, you are suggesting that I should include this into the core lib, right? We can definitely do that and expose this into a parameter!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that makes sense, as the desire to try to reconcile the two images into the same coordinate system is going to be a pretty common use case!

@rexagod
Copy link
Member

rexagod commented Jul 17, 2019

Sure, @jywarren! Just a question though, should I stall this one a bit and first complete work on the image-sequencer module then start work here, or would you like me to look into this at the same time? I'm fine with whatever you say, so let me know! Thanks!

@@ -1,3 +1,9 @@
body {
Copy link
Member

Choose a reason for hiding this comment

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

we have started using more specific css selectors in our library so they don't overwrite anything downstream. see #330

@jywarren
Copy link
Member Author

jywarren commented Feb 8, 2020

Noting that the promise code noted by @rexagod should now be easier to integrate due to webpack adoption in #512 (comment)

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.

3 participants