Adding option for visibility #133
Conversation
ogre/src/OgreLidarVisual.cc
Outdated
| // The newly created dynamic lines are having default visibility as true. | ||
| // The visibility needs to be set as per the current value after the new | ||
| // renderables are created. | ||
| this->SetVisible(this->Visible()); |
There was a problem hiding this comment.
This is a little bit weird. Are you setting the variable with the letter? I think it's because this line, right?
T::SetVisible(_visible);
There was a problem hiding this comment.
I'm sorry I don't understand the question.
I did the above as the visibility changes once the newly created DynamicRenderables are added to the LidarVisual, the default value of the visibility of the newly created renderable is true. Hence I am setting the current value of the visibility again to make sure that the newly added DyanmicRenderables do not have a separate value from the intended current value. On deleting the old ones and creating the new ones (which happens either during changing of visual type or changing the sensor topic or setting DisplayNonHitting false) it used to be visible even though the checkbox was off.
There was a problem hiding this comment.
lets do minimal changes with this PR since we're trying to wrap up the work. We would need to add tests for the new SetVisible / Visible functions and make sure visibility inheritance also works but I think that's out of scope of what we are trying to fix. Instead, can you add the SetVisible function in OgreLidarVisual and Ogre2LidarVisual and add the visible variable in their private class to track their visibility (and remove the ones in the base classes)? So on every Update call we would just do SetVisible(this->dataPtr->visible)
|
|
||
| /// \brief Set visibility of the visual | ||
| /// \param[in] _visible Visibility of the visual | ||
| public: virtual void SetVisible(bool _visible) = 0; |
There was a problem hiding this comment.
LidarVisual should inherit the SetVisible function from Visual class already so you should not have to add a new one here
ogre2/src/Ogre2LidarVisual.cc
Outdated
| // The newly created dynamic lines are having default visibility as true. | ||
| // The visibility needs to be set as per the current value after the new | ||
| // renderables are created. | ||
| this->SetVisible(this->Visible()); |
There was a problem hiding this comment.
I think you can move this call out side of the for loop, e.g. at the end of the function? Same for OgreLidarVisual class
Signed-off-by: Mihir Kulkarni <[email protected]>
mihirk284
force-pushed
the
lidar_visibility_fix
branch
from
77bb530 to
bf233c8
Compare
@iche033 Please see this PR to solve the issue in ign-gazebo/#301
I have added to the API to set and get the visible state of the visual. I could not access the existing visibility status of the ogre node as they have not provided API's for the same. [link]