-
Notifications
You must be signed in to change notification settings - Fork 28
PixelObjectList implementation - Duplicates by visual identification #79
base: master
Are you sure you want to change the base?
Conversation
…tial list for matching later on.
…yzed on a pixelobject level.
…of fuzzy logic, but it will have to do. Eventually, these fuzzy logic sections should be user-programmable.
… replacing it with functions within a namespace.
…, write atest to see if PixelObjectList works.
… recognition using PixelObjectList is programmed. The fuzzy logic still needs to be fine tuned and modified. An integration test has been written for unique object detection.
…leted to reduce processing time. At the current rate, a full flight of objects will require over 8 billion minutes to process (and who knows how much memory). This algorithm may need to be optimized a bit more, however human vision interaction may be necessary to garbage collect objects which have no importance or significance to the overall mission. Also added target analysis functionality into main.cpp.
…ges to pixel_object_list and supporting functions. Expanded on fuzzy logic and hieretical analysis
…ed some less important functions, that currently served no purpose.
benjaminwinger
left a comment
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 looks pretty good in general, though it seems a little incomplete. You should probably focus on getting the GPS stuff working and testable, since I imagine we'll be mainly be relying on clustering by GPS co-ordinates.
modules/core/include/object.h
Outdated
| * | ||
| * @return The distance covered by each pixel of the image in the X and Y | ||
| * directions. | ||
| */ |
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.
Objects wouldn't have a single distance since they are composed of multiple frames. If anything you might want to include this sort of information in a frame.
modules/core/include/target.h
Outdated
| * @param y The distance covered by each pixel of the image in the Y. | ||
| */ | ||
| void set_pixel_distance(double x, double y); | ||
| void set_pixel_distance(cv::Point2d); |
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.
Same as the comment on Object, Target likewise isn't tied to a single frame
| } | ||
| //Is the best match good enough - FUZZY LOGIC | ||
| //TODO: This should be a configurable value via a socket connection or a | ||
| //learning algorithm |
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.
Just a thought here, but we should create a settings singleton for this purpose. Then the settings object can handle exposing a way to be modified that we can use through the GUI.
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.
Little slow on this PR. This TODO would be something I'll try and include in a separate PR.
| * | ||
| * @return vector containing the unique Targets which have been analyzed | ||
| */ | ||
| void getGPSCorners(cv::Point2d cameraAlpha, double longitude, double latitude, |
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 should probably operate on a just a Frame. We would need to include to include a reference to camera information (including the alpha) in Frames. For now probably just add the cameraAlpha to the Frame class, we'll have to add support for camera profiles of some sort later.
You'd be able to get the lat/lon/alt/heading too from the frame's Metadata struct.
Also, unit tests would be great for the GPS calculations.
I don't know if we've talked about this before, but assume that at this point the image is rectified, later we'll have to rectify Frames when we first create them.
I'm not sure what we should do about perspective distortion though. The stock GoPro lens is really wide, so even with correcting for lens distortion, the perspective distortion will still mean that the edges of the frame are going to quite compressed compared to the centre, meaning that using the corners of the image to interpolate the position of a given point may be hit and miss (if that was what you were planning on using this for). I think going from the centre should give better results in general.
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.
Good point about using the center rather than corners. Using something similar to "pixel distance" will be used to determine other points based on the center point.
| return 0; | ||
| } | ||
|
|
||
| double PixelObjectList::compareContours(PixelObject* po1, Object* o2){ |
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.
Given how slow clustering can be already, I'd sort of prefer to be comparing precomputed data. I think it should be sufficient to compare precomputed information such as the regularity of the contour, largest and smallest diameter, etc..
If anything we could start with comparing precomputed information and directly compare contours to double check clusters.
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.
Fixed.
| } | ||
|
|
||
| void Object::add_pobject(PixelObject * po){ | ||
| pixelObjects.push_back(po); |
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.
Note that there's a bunch of other things that need to be done here. For now just averaging the data from the PixelObjects in the Object should be fine, we can do more later.
…coordinate math, followed by threshold analysis.
… Added unit test to check calculations. For now, wide angle go-pro alpha angle values are being used. Exact calibration should be done manually.
|
@benjaminwinger Issues fixed with respect to your comments. Two things that are not in this PR and will be worked on in a future one:
-GPS code works in unit tests, I just need a proper camera calibration (alpha values) and to test it in the field or with videos. |
…ects together is very iffy. It needs to be tuned using a GUI interface (AKA Human Vision).
…nd threshold_bias values for the pixel_object_list analysis.
|
@benjaminwinger If you get the chance, can you review this PR? |
|
Sorry, I've been a little busy (and sick). |
|
Sorry no worries. I'm going to try and get a live version of this running this weekend though... |
benjaminwinger
left a comment
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've added a few comments. I'm going to have to look at this more tomorrow.
modules/core/include/object.h
Outdated
| * positions/colours/areas with potential error | ||
| * | ||
| */ | ||
| void update(); |
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 note that you only use this in conjunction with add_pobject (and vice versa). Why not combine the two methods?
| * @param o the pixelobject that is converted to an object and added to the | ||
| * list. | ||
| */ | ||
| void addNode(PixelObject* o); |
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.
Is there a situation when this needs to be called externally? I would imagine that you only want to be calling addAndCompare. The same goes for the comparison functions, wouldn't they only be used internally?
I don't think there is any reason for them to be public.
modules/core/include/object.h
Outdated
| * @file object.h | ||
| * @author WARG | ||
| * | ||
| * @brief Container class for storing information about |
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 describes a class not a file. I'm not really sure why this needed to be moved.
| * possible error. As targets are processed unique targets will | ||
| * be identified and the data combined into a single object. | ||
| * | ||
| * This class is a singleton. |
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.
Does this really need to be a singleton? I would think that you only want the TargetAnalyzer singleton to be accessible globally. You could just give the TargetAnalyzer singleton an instance of the PixelObjectList.
Also, why have a special data structure for the PixelObjects? Wouldn't it work to do the comparisons inside the TargetAnalyzer and use an STL data structure to store the objects?
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 works right now. I'll make it a non-singleton, but I'm not going to go through and modify the List with the standard library.
I'll fix this later is what I am saying. (AKA low priority)
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.
Understandable. Abstracting away the data structure used would make it easier to make it thread-safe, but I think the best thing to do for now is just to run this in a single thread anyway. That would mean buffering the pixel_objects to process and creating a new worker thread that reads from the buffer and handles them (adding to the buffer is technically still not entirely thread-safe since most STL data structures don't support concurrent writers, but a lot better than modifying the PixelObjectList from 8 threads at once. Boost has some good thread-safe containers that should be looked into at some point).
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.
Agreed. The slow part is still the TargetId stuff, so a single thread should be okay...
You are right multiple threads in list comparisons could get a little bit tricky.
IE: Object 1 and object 2 are the same, but due to concurrency don't ever get properly compared and are both appended to the list as a unique object.
| //not it is a duplicate will be known | ||
| int i = 0; | ||
| while(i < listLength){ | ||
| tail->o-> |
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 clearly doesn't compile. The only reason there isn't an error is because this file isn't included in the CMakeLists.
Then again, is the ObjectList really necessary since the PixelObjectList is handling Objects?
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 will remove this file.
benjaminwinger
left a comment
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 looks pretty good overall from a code point of view.
Hopefully we can test it's effectiveness some time before the competition (I'll try to put the camera stuff up tomorrow so that we can get more accurate GPS co-ordinates)!
| cv::Scalar cSum; | ||
| for (PixelObject* po : pixelObjects){ | ||
| this->centroid += po->get_gps_centroid(); | ||
| this->area += po->get_gps_area(); |
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 isn't being updated afterwards to make it an average instead of a summation
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.
Fixed now.
| this->centroid = cv::Point2d(0,0); | ||
| cv::Point2d maxDistance(0,0); | ||
| cv::Scalar cSum; | ||
| for (PixelObject* po : pixelObjects){ |
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.
Noting my other comment about combining this with adding objects, when adding objects these values could be updated rather than re-calculated.
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 I will rename this function to "recalculate" and have another called "update". Both will be private to the class.
| return 0; | ||
| } | ||
| po1->set_gps_centroid(result); | ||
| double distance = TargetAnalyzer::getInstance()->getGPSDistance(result, o2->get_centroid()); |
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 worry a bit about calculations like this in that the result is possibly going to vary quite a bit depending on the order that targets are added. Unfortunately it's probably the best we can do for now given that it would get a lot more complicated to do otherwise.
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.
Agreed. A better solution would be to store all GPS coordinates and then use K-Means clustering....probably won't happen before competition.
|
|
||
|
|
||
| //TODO: ADD OPTION TO ONLY LOOK AT PREDEFINED SHAPES | ||
| double PixelObjectList::compareContours(PixelObject* po1, Object* o2){ |
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.
How slow is this actually? You had said before that it wasn't terribly fast, and it's just going to get a lot slower the more objects are in the list.
That is to say, after 20 minutes in the air, can we expect this to be running at a remotely usable speed? I think it's safe to assume we may have as many as several hundred pixelobjects detected during the flight and every time a new one is added it will be compared to each of the previous ones in turn. If it takes a few minutes to add a each one by the end then I don't think it's worth using this as it is at all.
I think I've suggested before just using precalculated statistics on the shape such as the shortest and longest real world distances across the contour through the centre, maybe some sort of regularity calculation too.
At the same time, if it is actually fast enough to be usable, it might be prudent to leave it be for now, given that we don't have much time.
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.
On my laptop 75 Pixel Objects take 90 seconds to compute. It is slow. I will use precomputed statistics instead from images I got from last flight test.
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 not sure how to hard code it. I'm thinking of using the target_loader, I partly made last year....to load a file with contour points.
What do you think? @benjaminwinger
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.
No, not hard-coded shapes, if we want to be filtering based on shape we can do that in the identification phase.
Here the point would be to cluster based on shape, so you'd calculate certain statistics, as mentioned before, during the identification phase and then here we they can be compared similarly to how you're comparing colour or location.
| BOOST_LOG_TRIVIAL(debug) << "Images in test: " << numImage; | ||
| BOOST_LOG_TRIVIAL(debug) << "PixelObjects in test: " << numPixelObjects; | ||
|
|
||
| BOOST_WARN_MESSAGE(numPixelObjects == 75, "Unexpected number of pixel objects, has the target id algorithm changed? Are there more images than expected?"); //If the number of test photos has changed, this integration test may become invalid |
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.
Isn't unit testing fun...
You don't need to change this now, but it would be better to feed synthesized data into the TargetAnalyzer so that the results are more predictable. You don't want this test to be failing just because the identification algorithm has changed.
|
Also, regarding the duplicate comments from yesterday (now deleted), the first time I accidentally closed the page and they disappeared, then my graphics driver crashed and I had to reboot my computer. |
main.cpp
Outdated
| boost::asio::io_service::work work(ioService); | ||
| processors = boost::thread::hardware_concurrency(); | ||
|
|
||
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.
In all honesty, why do editors even allow you to create lines like 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.
Woops. Any vim extension suggestions?
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.
See http://vim.wikia.com/wiki/Remove_unwanted_spaces
What I use:
func! DeleteTrailingWS()
exe "normal mz"
%s/\s\+$//ge
exe "normal `z"
endfunc
autocmd BufWrite *.{c,h,cpp,hpp,rs,java,py,etc...} :call DeleteTrailingWS()
The BufWrite glob can be modified depending on the files you use. Note that it isn't actually good to do this for all files. Try programming with Whitespace when doing so for example (why you would actually use Whitespace is an entirely different question).
It also means that everything you touch gets whitespace stripped, and depending on what codebase you're working on, people can get annoyed if you start checking in changes where every file that you touch has loads of whitespace changes. I generally just use git add -p and avoid other people's whitespace issues.
…tude faster and provides very similar results. Computation of length to width ratio, area to perimeter ratio, actual area to square area, etc.
This first PR only looks at the visual portion of the analysis.
Note there are some preliminary GPS functions and colour functions for now. They are a result of planning. They are included in this PR, but are not in a functional state. Please ignore these mostly empty functions.
This PR should have been completed a while ago....here it is.