Visualze Frustum #2707
Visualze Frustum #2707
Conversation
Signed-off-by: Utkarsh <[email protected]>
BA-Utkarsh
force-pushed
the
gz-sim9-v1
branch
from
32d5e9b to
cbabb37
Compare
Signed-off-by: Utkarsh <[email protected]>
|
@ahcorde , thank you for review. I have addressed all the review comments. |
|
Thanks for the contribution. There are still stray spaces in the code. Do you mind fixing them: https://github.com/gazebosim/gz-sim/actions/runs/12514253900/job/35270143157?pr=2707 Also is there a chance that we can visualize the frustum without the need for pressing play? |
Signed-off-by: Utkarsh <[email protected]>
Signed-off-by: Utkarsh <[email protected]>
| { | ||
| std::lock_guard<std::mutex> lock(this->dataPtr->serviceMutex); | ||
| this->dataPtr->frustum->SetVisible(_value); | ||
| gzerr << "Frustum Visual Display " << ((_value) ? "ON." : "OFF.") |
There was a problem hiding this comment.
is it really error level of logging?
| } | ||
| std::string childname = std::string( | ||
| comp->Data()); | ||
| if (nextstring.compare(childname) == 0) |
There was a problem hiding this comment.
nextstring and childname are std::strings, why not use == for it?
Like: if (nextstring == childname)
| { | ||
| std::vector<transport::MessagePublisher> publishers; | ||
| this->dataPtr->node.TopicInfo(topic, publishers); | ||
| for (auto pub : publishers) |
There was a problem hiding this comment.
do you really need a pub by copy? Just curious
| // Get updated list | ||
| std::vector<std::string> allTopics; | ||
| this->dataPtr->node.TopicList(allTopics); | ||
| for (auto topic : allTopics) |
There was a problem hiding this comment.
maybe const reference could be enough for topic?
| { | ||
| auto frustumURIVec = common::split(common::trimmed( | ||
| this->dataPtr->frustumString), "::"); | ||
| if (frustumURIVec.size() > 0) |
There was a problem hiding this comment.
size() is already unsigned, maybe !frustumURIVec.empty() is more c++-like
(and for some containers size() operation can be quite costly)
| bool foundChild = false; | ||
| for (auto child : children) | ||
| { | ||
| std::string nextstring = frustumURIVec[i+1]; |
There was a problem hiding this comment.
it can be const reference on a left side of expression, you don't need a full string copy here AFAIS
const std::string &nextstring = frustumURIVec[i+1]; or use auto instead of std::string
|
|
||
| this->dataPtr->visualDirty = true; | ||
|
|
||
| for (auto data_values : this->dataPtr->msg.header().data()) |
There was a problem hiding this comment.
const reference should be fine here I suppose
| if (this->dataPtr->frustumString.compare( | ||
| common::trimmed(data_values.value(0))) != 0) |
There was a problem hiding this comment.
seems it's comparing of 2 strings, seems better to use != here
| } | ||
| } | ||
| } | ||
| if (this->dataPtr->topicList.size() > 0) |
There was a problem hiding this comment.
same comment as above about size() and empty
|
@ntfshard , \cc: @ahcorde In my opinion, the changes are not needed for now. @ahcorde , please let me know your views on the same.. |
iche033
added
the
needs upstream release
|
@mjcarroll , please let us know your view as well.. |
|
@mjcarroll @ahcorde @iche033 any update? |
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Signed-off-by: Utkarsh <[email protected]>
|
Signed-off-by: Utkarsh <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Co-authored-by: Ian Chen <[email protected]> Signed-off-by: Utkarsh Yenurkar <[email protected]>
Thank you very much @iche033 for your support.
Surely, will create a separate PR after this merge.
Oh great! |
🎉 New feature
Summary
This PR mainly adds the visualization of Frustum.
We could see it was present in gazebo classic and from gazebo garden onwards the plugin/feature is not available.
Test it
$ Build gazebo from source.
$ . install/setup.sh
$ gz sim examples/worlds/visualize_frustum.sdf
Test Ref images,
Play the simulation.
Select the topic from scroll down.
Refresh it to get the "logical_camera/frustum" topic.
Subcriibed to "logical_camera/frustum".
Final output
Checklist
codecheckpassedSupporting PRs,