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

Added LightVisual #202

Merged
merged 13 commits into from
Feb 3, 2021
Merged

Added LightVisual #202

merged 13 commits into from
Feb 3, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jan 20, 2021

This PR adds LightVisual. It's related with this issue gazebosim/gz-sim#193

Ogre2 is working fine:

Selección_090

But in Ogre I'm only able to see the LightVisual attached to an object.

Selección_091

When I run the simple_demo which includes some lights I'm able to see it in both cases:

  • Ogre

Selección_092

  • Ogre2
    Selección_093

@iche033, any idea why in Gazebo the LightVisual is not being rendered ?

Signed-off-by: ahcorde [email protected]

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde added the 🏢 edifice Ignition Edifice label Jan 20, 2021
@ahcorde ahcorde requested a review from iche033 January 20, 2021 11:09
@ahcorde ahcorde self-assigned this Jan 20, 2021
Signed-off-by: ahcorde <[email protected]>
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #202 (a4f25d0) into main (55498bf) will increase coverage by 3.97%.
The diff coverage is 68.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
+ Coverage   53.01%   56.99%   +3.97%     
==========================================
  Files         143      151       +8     
  Lines       13600    14988    +1388     
==========================================
+ Hits         7210     8542    +1332     
- Misses       6390     6446      +56     
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
ogre/src/OgreCamera.cc 0.00% <0.00%> (ø)
ogre/src/OgreLightVisual.cc 0.00% <0.00%> (ø)
ogre/src/OgreVisual.cc 27.55% <28.57%> (+27.55%) ⬆️
include/ignition/rendering/base/BaseLightVisual.hh 30.09% <30.09%> (ø)
ogre/src/OgreScene.cc 28.46% <44.44%> (+28.46%) ⬆️
ogre2/src/Ogre2Scene.cc 88.70% <44.44%> (-1.74%) ⬇️
ogre2/src/Ogre2LightVisual.cc 72.34% <72.34%> (ø)
ogre/src/OgreHeightmap.cc 72.70% <72.70%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dbdef4...a4f25d0. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Jan 27, 2021

@iche033, any idea why in Gazebo the LightVisual is not being rendered ?

turns out to be an issue on the ign-gazebo side. We are not attaching the light to the scene. By default ogre2 attaches a light without parent to the root but ogre1.x does not and we have to manually attach it to the scene tree ourselves. Here's a patch to make light visuals appear in ign-gazebo:

diff --git a/src/rendering/SceneManager.cc b/src/rendering/SceneManager.cc
index 6aa73f78..1377def0 100644
--- a/src/rendering/SceneManager.cc
+++ b/src/rendering/SceneManager.cc
@@ -926,6 +926,8 @@ rendering::LightPtr SceneManager::CreateLight(Entity _id,
 
   if (parent)
     parent->AddChild(light);
+  else
+    this->dataPtr->scene->RootVisual()->AddChild(light);
 
   return light;
 }

@iche033
Copy link
Contributor

iche033 commented Jan 27, 2021

Can we move the code for creating the light visual to ign-gazebo gui plugin? So instead of always creating the LightVisual when a Light is created, we create them in ign-gazebo only when the users requests to visualize them. Similar to the idea of lidar visualization (and we can then also toggle their visibility on/off). These visualizations are gui only so we don't need to always create them.

/// \brief Enum for LightVisual types
enum IGNITION_RENDERING_VISIBLE LightVisualType
{
LightVisual_NONE = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

add /// \brief

Copy link
Contributor

@iche033 iche033 Jan 29, 2021

Choose a reason for hiding this comment

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

can you also change the naming convention of these enums to match others in ign-rendering?
e.g.
https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering4/include/ignition/rendering/Marker.hh#L38

so this would be something like LVT_NONE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a conflict with LidarVisualType. Any recomendations ?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that's unfortunate, how about LVT_EMPTY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iche033 Resolved! 69232c0

@ahcorde ahcorde requested a review from iche033 February 1, 2021 12:46
Signed-off-by: Ian Chen <[email protected]>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

I added some doc in a4f25d0. Changes look good. I think windows build failure is an issue with the CI machine

@ahcorde ahcorde merged commit f9b58e6 into main Feb 3, 2021
@ahcorde ahcorde deleted the ahcorde/feature/LightVisual branch February 3, 2021 06:48
sloretz added a commit to sloretz/ign-rendering that referenced this pull request Feb 3, 2021
This reverts commit f9b58e6.

Signed-off-by: Shane Loretz <[email protected]>
@peci1
Copy link
Contributor

peci1 commented Mar 23, 2021

@ahcorde can you check spotlights do not suffer from the same bug as Gazebo Classic doubling their FOV? gazebosim/gazebo-classic#2947 . Looking at the code it seems to me the same bug was copied...

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 21, 2021

@peci1 FYI: PR #308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants