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

Adds Python bindings for the Light convenience class #2043

Merged
merged 31 commits into from
Aug 25, 2023

Conversation

Voldivh
Copy link
Contributor

@Voldivh Voldivh commented Jul 19, 2023

🎉 New feature

Summary

This PR adds bindings for the Light convenience class.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@Voldivh Voldivh marked this pull request as ready for review July 19, 2023 20:13
@Voldivh Voldivh requested a review from mjcarroll as a code owner July 19, 2023 20:13
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

tests?

python/src/gz/sim/Light.cc Outdated Show resolved Hide resolved
python/src/gz/sim/Light.cc Outdated Show resolved Hide resolved
python/src/gz/sim/Light.cc Outdated Show resolved Hide resolved
python/src/gz/sim/Light.cc Outdated Show resolved Hide resolved
python/src/gz/sim/Light.hh Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #2043 (49ac517) into main (d409496) will decrease coverage by 0.06%.
The diff coverage is 45.97%.

❗ Current head 49ac517 differs from pull request most recent head f0732eb. Consider uploading reports for the commit f0732eb to get more accurate results

@@            Coverage Diff             @@
##             main    #2043      +/-   ##
==========================================
- Coverage   65.35%   65.30%   -0.06%     
==========================================
  Files         320      321       +1     
  Lines       30216    30303      +87     
==========================================
+ Hits        19749    19789      +40     
- Misses      10467    10514      +47     
Files Changed Coverage Δ
python/src/gz/sim/Link.cc 50.00% <ø> (ø)
python/src/gz/sim/Light.cc 45.34% <45.34%> (ø)
python/src/gz/sim/_gz_sim_pybind11.cc 100.00% <100.00%> (ø)

Base automatically changed from nkoenig/7-to-main to main July 20, 2023 18:06
@Voldivh Voldivh changed the base branch from main to voldivh/python_bindings_world July 27, 2023 15:38
@Voldivh Voldivh requested a review from mabelzhang as a code owner July 27, 2023 15:38
@Voldivh Voldivh force-pushed the voldivh/python_bindings_light branch from bde5acb to ebab22a Compare July 27, 2023 15:44
@Voldivh
Copy link
Contributor Author

Voldivh commented Jul 27, 2023

@azeey The setters for this class doesn't seem to be working for some reason I'm not able to find. The test I added shouldn't be passing after I set the property to another value and they do. Also there are other values that doesn't change from the default value even after defining it on the sdf file such as the spot_inner_angle, spot_outer_angle and spot_falloff.

<sdf version="1.6">
<world name="world_test">

<light type="point" name="light_test">
Copy link
Contributor

Choose a reason for hiding this comment

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

The <direction> and <spot> parameters do not apply when the light type is "point" (see http://sdformat.org/spec?ver=1.10&elem=light).

Copy link
Contributor Author

@Voldivh Voldivh Jul 27, 2023

Choose a reason for hiding this comment

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

Oh I see, we have point, spot and directional. That makes a lot more sense. That should solve some of my problems, but, I think the setters's issue remains the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the required changes in 4e72d74. However, the behavior is the same as I expected, solved some issues but not a single set method is working.

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@Voldivh Voldivh force-pushed the voldivh/python_bindings_light branch from 95ee75f to fadc032 Compare August 1, 2023 18:00
@Voldivh Voldivh force-pushed the voldivh/python_bindings_light branch from fadc032 to d2cf8f6 Compare August 8, 2023 16:04
Base automatically changed from voldivh/python_bindings_world to main August 8, 2023 21:54
#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Voldivh can you address this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

the header is still there? did you push ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might have been re-added when bringing back an older commit for testing. Fixed in 801bb08

{
namespace python
{
/// Define a pybind11 wrapper for a gz::sim::World
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Define a pybind11 wrapper for a gz::sim::World
/// Define a pybind11 wrapper for a gz::sim::Light

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks and done.

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Voldivh can you address this?

python/test/light_TEST.py Show resolved Hide resolved
@azeey azeey changed the title Adds bindings for the Light convenience class Adds Python bindings for the Light convenience class Aug 22, 2023
@azeey azeey requested a review from ahcorde August 25, 2023 04:17
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

There is still a pending comment, good to go when the iostream header is removed

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

the header is still there? did you push ?

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey merged commit 8614184 into main Aug 25, 2023
4 of 6 checks passed
@azeey azeey deleted the voldivh/python_bindings_light branch August 25, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants