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

Break up #70 - visualization only, no lat/long to Cartesian conversion #93

Merged
merged 22 commits into from
Nov 29, 2021

Conversation

mabelzhang
Copy link
Collaborator

@mabelzhang mabelzhang commented Nov 25, 2021

Breaking up part of #70 into a smaller PR. This PR has all the visualizations, no lat/long to Cartesian conversion.

Looking at the files in #70, the only file that needs breaking up is lrauv_ignition_plugins/src/ScienceSensorsSystem.cc.
Most other files are GUI and VisualizePointCloud.cc, which @chapulina and I already looked at closely in #85, which has been merged into #70.

Diff of this PR from #70:

  • In buoyant_tethys.sdf, the data file is changed to simple_test.csv, since lat/long to Cartesian conversion isn’t taking place, we cannot test the actual data files.
  • A few parameters (MINIATURE_SCALE, dimFactor, RES_*) are tweaked to this test file, so that >0 markers show up and are big enough to see, with the unconverted lat/long. Bug fix on calculating renderEvery.

(I did a subtractive instead of additive diff 😅 I mean, it is Thanksgiving eve, and the only lines that needed deleting have to do with the lat/long to Cartesian conversion, which is <100 lines. Since we squash-merge, the diff shouldn't look weird in the commit history.)

To test

ign gazebo -v 4 buoyant_tethys.sdf

Unpause, enable Visualize Point Cloud plugin, click refresh button. Change the data type.
Check that for chlorophyll, 13 markers are visualized, because the 17 others are NaN. For salinity and temperature, 30 are rendered.

2021-11-25-000555_2400x2000_scrot

2021-11-25-000558_2400x2000_scrot

2021-11-25-000601_2400x2000_scrot

Bug

When switching from salinity (30 valid points) or temperature (30 valid points) to chlorophyll (13 valid points), there is a bug. More than 13 markers are rendered for chlorophyll. Debug printout says 30 rendered in the first round (but you see 25 in the scene), then the printout goes down to 13. But since no unsubscription is performed in between, you still see 25.

2021-11-25-000603_2400x2000_scrot

This could be a timing issue. When a new data type is subscribed, the new float array data may not have been received before it tries to render the markers, so it’s rendering the old data. One guess is this may have been introduced by changing to use a single array to represent all the data types. There are more edge cases that need to be taken care of when switching the data in that array (I saw some of these unclean cases when I was on a time crunch, which was the reason I went to multiple arrays).

We may not have noticed this before because the real data has too many points to really test this. The simple test shows this very clearly.

I tried tentatively moving the mutex in PublishMarkers(), to see if it’s a lucky hit since nPts takes the size of the point cloud. But that didn’t fix it.

@chapulina could you help looking into this behavior?

mabelzhang and others added 21 commits November 2, 2021 00:32
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM. A lot of the optimizations introduced here will be removed in #88, which is changing the boxes to points.

this->dataPtr->PublishData();
}
}

// Publish every n iters so that VisualizePointCloud plugin gets it.
// Otherwise the initial publication in Configure() is not enough.
if (this->dataPtr->repeatPubTimes % 10000 == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should revisit this, the service call that VisualizePointCloud makes at startup is meant to address the case when it misses the initial publish from this system, or when that plugin is simply loaded later.

// TODO optimization for visualization:
// Use PCL methods to chop off points beyond some distance from sensor
// pose. Don't need to visualize beyond that. Might want to put that on a
// different topic specifically for visualization.
Copy link
Contributor

Choose a reason for hiding this comment

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

When we use points for visualization, we're able to comfortably display hundreds of thousands of points at once, see #88 . So I think this optimization may not be necessary. There may also be use cases for visualizing the entire dataset, even points that are far from the robot. We can leave this TODO here until #88.

@chapulina
Copy link
Contributor

The PitchMass test failed, but I think it's unrelated to this issue, see #97

@chapulina chapulina merged commit c26260b into main Nov 29, 2021
@chapulina chapulina deleted the mabelzhang/science_viz_part1 branch November 29, 2021 20:56
@mabelzhang
Copy link
Collaborator Author

Thanks for merging this quickly.

Did you see the behavior in the Bug section above? I want to make sure you are aware and see if you can reproduce it with the simple_test.csv. We are moving very quickly to the real data set, but because this happened, I think we might want to test with the simple_test.csv more rigorously.

@chapulina
Copy link
Contributor

Did you see the behavior in the Bug section above?

I couldn't reproduce it, but I didn't try too much. I've been testing more with #88 and I don't see the issue. It may be due to the change from an array of markers to a single marker.

@mabelzhang
Copy link
Collaborator Author

mabelzhang commented Dec 3, 2021

Hmm I will wait to test on #88 after all its dependent PRs are in Ignition.

As an updated data point, I can still reproduce the bug with #98.
This is the procedure from startup, quite straight-forward and reliable on my end. See all the extra points for chlorophyll that aren't there in the first screen, but are there in the 4th screen:

2021-12-03-02-29_visualizationUnsubscribeBug.mp4

If you can't reproduce it, maybe I can clean my workspace and try again, or wait for #88. I don't need it yet, until all the interpolation stuff is in.

@chapulina
Copy link
Contributor

Ah ok, thanks for the video, I can reproduce it on main. Here are some fixes on main while we wait for #88:

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.

2 participants