Add get3DPatch to ViewPicker public API #657
Conversation
schornakj
force-pushed
the
feature/view-picker-get3Dpatch
branch
3 times, most recently
from
8fdd91c
to
9f25d21
Compare
| uint8_t c = data_ptr[4 * pixel + 2]; | ||
| uint8_t a = data_ptr[3 * pixel]; | ||
| uint8_t b = data_ptr[3 * pixel + 1]; | ||
| uint8_t c = data_ptr[3 * pixel + 2]; |
There was a problem hiding this comment.
Interesting. I took a look through the code, and I think we expect three values per pixel because of the format set here:
It's possible in ROS 1, the format included an alpha channel, hence 4 bytes. It might be a good idea to assert that the pixel box data is three-channel, prior to extracting values. Could you try adding the following before this loop?
assert(Ogre::PF_R8G8B8 == depth_pixel_box_.format);It might be worth leaving that in there just in case, or at least referencing in a comment where this format is coming from.
There was a problem hiding this comment.
I added this assert and an explanatory comment in #661
There was a problem hiding this comment.
I was trying to figure out where this actually changed as well, unfortunately without much success so far. The depth_render_texture_ is initialized with the same PF_R8G8B8 enum value in Melodic and Noetic RViz:
depth_render_texture_ = Ogre::TextureManager::getSingleton().createManual(
tex_name, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME, Ogre::TEX_TYPE_2D,
depth_texture_width_, depth_texture_height_, 0, Ogre::PF_R8G8B8, Ogre::TU_RENDERTARGET);
9f25d21
to
308f99c
Compare
schornakj
changed the title
schornakj
force-pushed
the
feature/view-picker-get3Dpatch
branch
from
308f99c
to
08d0ca2
Compare
I moved the bug fix to a new PR at #661. That's definitely a more sensible way to arrange things -- I was only thinking about getting these functions working in order to make my code work! |
ViewPicker::get3DPatch is currently a private member function of the ViewPicker class. It would be very useful to make it public, since this lets us develop plugins that can use the orientations of mesh surfaces and point clouds in the 3D scene. This change moves it from the private API to the public API. Signed-off-by: Joe Schornak <joe.schornak@gmail.com>
schornakj
force-pushed
the
feature/view-picker-get3Dpatch
branch
from
08d0ca2
to
6466b2b
Compare
ViewPicker::get3DPatchis currently a private member function of the ViewPicker class. It would be very useful to make it public, since this lets us develop plugins that can use the orientations of mesh surfaces and point clouds in the 3D scene. This change moves it from the private API to the public API.Additionally, the method in which OGRE packs depth pixel data has changed in the version currently used by RViz2. In ROS Melodic RViz eachuint32pixel was represented by 4 sequentialuint8elements in an array, but only the first 3 elements actually contained useful data and the 4th element was discarded. In the current version (I'm testing with Foxy right now) it looks like each depth pixel is now represented by just 3 sequentialuint8elements, without the 4th "empty" index. This means that the previous method of accessing the data for each pixel by index relative to the start of the array no longer works correctly and returns mangled or misaligned data for all pixels after the first. A change to the data pointer indexing method in theViewPicker::getPatchDepthImagefunction was needed to correctly calculate distances from the scene camera when getting patches containing more than one pixel.The data indexing bug did not cause problems previously because it does not affect the depth value for the first pixel in the array.ViewPicker::getPatchDepthImageis only used byViewPicker::get3DPatch, which is itself only used byViewPicker::get3DPointto get a single-pixel patch. This issue is also somewhat challenging to demonstrate, since I think the current RViz test suite only uses mock versions of these functions.#661 now contains the pixel byte indexing fix described above which was originally part of this PR.
Signed-off-by: Joe Schornak joe.schornak@gmail.com