-
Notifications
You must be signed in to change notification settings - Fork 253
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
Plugin to spawn lights #587
Conversation
Signed-off-by: ahcorde <ahcorde@gmail.com>
you mean the light visuals? I think it's fine to leave them out
I see this when spawning a directional light. It could be that we can't have more than one directional lights in the scene. If that's the case, we can print a warning that only 1 directional light is supported for now
yes you can attach LightVisual to Light. Since the they are all ign-rendering nodes, you can just do |
Signed-off-by: ahcorde <ahcorde@gmail.com>
@iche033 I improved the implementation. The issue with the lights is the following: When point lights and directionals are mixed the segfault appears |
Signed-off-by: ahcorde <ahcorde@gmail.com>
ok narrowed the the point vs directional light issue a bit more. Disabling the point light shadows makes them work now:
|
Signed-off-by: ahcorde <ahcorde@gmail.com>
@iche033 It's ready for a review, thank you for the catch with the lights.
|
src/SdfEntityCreator.cc
Outdated
else if (_light->Type() == sdf::LightType::SPOT) | ||
{ | ||
this->dataPtr->ecm->CreateComponent(lightEntity, | ||
components::LightType(std::string("Spot"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about creating a convert
function from sdf::LightType
to std::string
in Conversions.hh
? It would be better to reuse the same logic in many places. I'm concerned that different plugins may use different strings (i.e. spot
, SPOT
...).
I can even see this function living in libSDFormat.
Also, I think that we should store the types as lowercase. The GUI can capitalize them as needed.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
* light visual as child of light Signed-off-by: Ian Chen <ichen@osrfoundation.org> * remove comment Signed-off-by: Ian Chen <ichen@osrfoundation.org>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@iche033, I addressed the comment from @chapulina 2a79677 |
Signed-off-by: ahcorde <ahcorde@gmail.com>
…otics/ign-gazebo into ahcorde/spawn/ligths
Signed-off-by: ahcorde <ahcorde@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great with the event change we talked about.
Besides the comment below, I have 2 requests:
- We should add the
Lights
plugin to the top toolbar next to the shapes in gui.config. - Now that the lights have a visual, most example worlds will have a green square near the origin for the sun. This may confuse users. On Gazebo classic, I've seen many users confused about the floating green square, imagine if it's on the ground 😅 I suggest we move all suns in the examples to z=10, so it's in a more "sun-like" position. I think it's ok if this comes in a future PR though.
src/rendering/RenderUtil.cc
Outdated
|
||
// create a new id for the light visual | ||
auto timeout = 100000u; | ||
for (auto i = 0u; i < timeout; ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to block the render thread. If this may take time to be completed, I suggest we queue it and process it when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the right name for this variable is attempts
, this loop has no relation with time or sleep. It's looking for an available entity ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh ok understood! I wonder if there's a chance of entity ID collision when new entities are inserted into the simulation from the backend. I didn't notice problems in my manual testing earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this method I can't access the ECM to call CreateEntity
. Is there any other way to get an empty ID ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the entity preview when spawning, there's this method in the Scene3D
plugin:
That plugin isn't accessible from RenderUtil
. But I think it could make sense to move the light visual logic from RenderUtil
to Scene3d
. The light visuals are only supposed to show on the client scene, not on the server. But RenderUtil
is used by both scenes. I think this would be a big change and it may take longer to merge this PR though 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can open an issue and merge this PR. Waht do you think @chapulina ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
friendly ping @chapulina
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue sounds good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue created #633
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina I made a quick review in the example folder. The suns' poses are already <light type="directional" name="sun">
<cast_shadows>true</cast_shadows>
<pose>0 0 10 0 0 0</pose>
<diffuse>1 1 1 1</diffuse>
<specular>0.5 0.5 0.5 1</specular>
<attenuation>
<range>1000</range>
<constant>0.9</constant>
<linear>0.01</linear>
<quadratic>0.001</quadratic>
</attenuation>
<direction>-0.5 0.1 -0.9</direction>
</light> |
Signed-off-by: ahcorde <ahcorde@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina I fixed the two suggestions: Feel free to squash and merge it |
Failing tests are unrelated:
Merging! |
I created a plugin to spawn lights in ignition-gazebo.
I have some questions and issues:
LightVisual
? Right now it's in theRenderUtil.cc
but when I remove the light from Gazebo in the component inspector the visual it's still there. Probably I should attach the Visual to the Light. Is this possible ?https://github.com/ignitionrobotics/ign-gazebo/blob/139e552a2fa2869794020313bea975143c7f273c/src/rendering/RenderUtil.cc#L404-L417
Signed-off-by: ahcorde ahcorde@gmail.com