Break up #70 - visualization only, no lat/long to Cartesian conversion #93
Conversation
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: Mabel Zhang <mabel@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
| 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) |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
|
The |
|
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 |
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. |
|
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. 2021-12-03-02-29_visualizationUnsubscribeBug.mp4If 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. |
|
Ah ok, thanks for the video, I can reproduce it on |
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:
buoyant_tethys.sdf, the data file is changed tosimple_test.csv, since lat/long to Cartesian conversion isn’t taking place, we cannot test the actual data files.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 calculatingrenderEvery.(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
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.
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.
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 sincenPtstakes the size of the point cloud. But that didn’t fix it.@chapulina could you help looking into this behavior?