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

Landmarks really observed #185

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ALouis38
Copy link
Contributor

Added options to the World and Camera views to visualize the landmarks seen by the current camera.
In each view, the display is triggered by a button. It uses the same options as the Landmarks menu.

It could also be interesting to fuse it with the Landmarks menu and transform it as a simple checkbox, but having a separate menu give more freedom on the display options (have size/colors or coloring method differ than the ones set for the landmarks).

@mleotta
Copy link
Member

mleotta commented Jul 20, 2016

This idea behind this branch is really useful, but I think it needs to be implemented differently. This is something @mwoehlke-kitware and I discussed a while back but never got around to implementing. I do feel that the UI needs to be unified the Landmarks menu. Having two top-level menus with the same icon is confusing, especially since some platforms only show the icon with no text. More importantly this feature needs to do more than just redraw landmarks seen from the current frame in a different color method. It needs to also not draw landmarks seen in the current frame with the other coloring method. Otherwise you have multiple points at the same point in space with different color fighting to be displayed and you don't know which one you will see, unless you can somehow guarantee which points are drawn on top.

I recommend a checkbox under Landmarks menu to color visible landmarks and a color selector to pick which solid color to use and possibly another size slider. We don't need the color by data or true color options for visible landmarks, the point is to just to highlight which landmarks are currently seen and a fixed color or larger size is best for that. When the box is checked, you'll need to draw the visible landmarks in the selected color and draw only the non-visible landmarks by the primary coloring parameters.

Also, finding which landmarks are visible on each frame is very inefficient and greatly slows down playback. The culprit here is MAP-Tk's current track data structure which is inefficient at the type of query you are trying to do. Until we fix that, I recommend building a lookup table for all frames that indicates which landmarks are visible on each frame and consulting that data structure when the frame index changes instead of scanning all the tracks again.

@ALouis38
Copy link
Contributor Author

I agree with this, but it may raise another issue. Right now, the Landmarks menu is accessible even when the Landmarks are not shown (same thing for the other menus). Thus, with a checkbox in this menu to display the visible landmarks, it may be possible to display the visible landmarks with the rest of the landmarks hidden. According to me, it's a good thing to be able to display the one without the other, but it might be confusing to have this option in a menu under a unchecked button.

@mleotta
Copy link
Member

mleotta commented Jul 21, 2016

@mwoehlke-kitware is the UI guy and maybe he has some thoughts on this. I agree that there may be value in showing only the visible landmarks and hiding the others. My first thought would be to add another UI element to enable this under the landmarks menu. Maybe it's another checkbox or maybe it's radio buttons or an option box, I don't know. But the choices are

  • display all landmarks (default)
  • display visible landmarks in a different color/size from the rest
  • display only visible landmarks and hide the rest

and of course all landmarks can be hidden by the top-level visibility toggle.

@ALouis38
Copy link
Contributor Author

Also, finding which landmarks are visible on each frame is very inefficient and greatly slows down playback. The culprit here is MAP-Tk's current track data structure which is inefficient at the type of query you are trying to do. Until we fix that, I recommend building a lookup table for all frames that indicates which landmarks are visible on each frame and consulting that data structure when the frame index changes instead of scanning all the tracks again.

I tried two solutions for this issue.

First, I tried to load every visible landmarks for each cameras at launch. It dramatically increases launch time, but there's no more loading to do once the application is started. You'll find this solution in commit cd3f2f7.

I then tested to load the visible landmarks for each active camera. When a camera get active and has no visible landmarks loaded, we load them. If we come back later to this camera, the visible landmarks are already loaded and they're directly displayed. This solution can be found in commit 8ece367.

The two solutions save the visible landmarks to a map. The only difference is when the map is populated (at launch or when a camera become active for the first time).

If you have a better solution to suggest, I'm all for it. I tried to optimize those ones but it's overall still slow, especially for the first one.

@mleotta
Copy link
Member

mleotta commented Jul 26, 2016

The third option, which @mwoehlke-kitware often points out to me, is to run this pre-caching in the background in a separate thread. I don't know how difficult that is to do. Maybe it is better left for a later branch.

For the first approach, you can make that more efficient by moving the inner loop over frame_id to inside the inner conditional (if (landmark)). You can assign the id, find the landmark, and check that it is valid one time before looping over the frame ids to populated the map.

For the second approach, you can make it more efficient by using track->find(id) instead of extracting all the ids and then using count to see if id is contained in the set.

I haven't tested either approach, but I suspect I would still prefer to compute this map once upfront. That is probably best done in a separate thread, but to keep it simple for now it could be done at load time, or maybe when the user first enables showing visible landmarks.

@ALouis38
Copy link
Contributor Author

For the second approach, you can make it more efficient by using track->find(id) instead of extracting all the ids and then using count to see if id is contained in the set.

I'll use this for now. I think using a thread for caching the visible landmarks is in fact a better solution, but I'd like to have some advices from @mwoehlke-kitware on the best way to do it. My intuition is to use QThread for that, but maybe there's a better way.

@mleotta
Copy link
Member

mleotta commented Jul 27, 2016

I've now tested the latest changes to this this branch. I like the new UI better, though for me I'm seeing some extra whitespace in the pop-up menu. Some items are aligned to the left and others to the right. So it looks a little odd.
screen shot 2016-07-27 at 9 42 36 am

Functionally the branch also seems to do the right thing except that it is really slow for larger data sets the first time you navigate through the frames (>1 second per frame). It's fine once the cache is computed. I think we really need to compute that cache in advance, even if it is a little slower at load time and not in a separate thread yet. It occurred to me that we must already be computing these visible landmark ids elsewhere in the GUI. The residuals feature that @mwoehlke-kitware added draws lines between corresponding features and the visible landmarks on each frame. So we should already know which landmarks are visible, but maybe that cached information is not exposed. I think it would be best if we could reuse information between these two features. Maybe @mwoehlke-kitware can elaborate further on this.

Also, I am seeing an occasional segfault when playing the video on this branch. I have not debugged yet to track down what is going on.

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.

None yet

2 participants