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.

Sign up for GitHub

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

Jump to bottom

Add Sky DOM #397

Merged
merged 7 commits into from
Nov 16, 2020
Merged

Add Sky DOM #397

merged 7 commits into from
Nov 16, 2020

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Oct 26, 2020

Add Sky DOM and tests

Signed-off-by: Ian Chen ichen@osrfoundation.org

@iche033
add sky dom
56d9b80 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🔮 dome Ignition Dome labels Oct 26, 2020
@codecov-io
Copy link

codecov-io commented Oct 26, 2020 •
edited

Codecov Report

Merging #397 into sdf10 will increase coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            sdf10     #397      +/-   ##
==========================================
+ Coverage   87.36%   87.50%   +0.14%     
==========================================
  Files          60       61       +1     
  Lines        9219     9325     +106     
==========================================
+ Hits         8054     8160     +106     
  Misses       1165     1165
Impacted Files Coverage Δ
src/Scene.cc 100.00% <100.00%> (ø)
src/Sky.cc 100.00% <100.00%> (ø)

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 050d78d...4fddd99. Read the comment docs.

chapulina
chapulina reviewed Oct 27, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Looking good to me, I just have some minor comments.

test/integration/scene_dom.cc Outdated Show resolved Hide resolved
include/sdf/Sky.hh Show resolved Hide resolved
include/sdf/Sky.hh Outdated Show resolved Hide resolved
include/sdf/Scene.hh Outdated Show resolved Hide resolved
src/Scene.cc Outdated Show resolved Hide resolved
src/Sky.cc Outdated Show resolved Hide resolved
src/Sky.cc Outdated Show resolved Hide resolved
@iche033
feedback changes
579b521 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033
Merge branch 'sky_dom' of github.com:osrf/sdformat into sky_dom
af94e71
@iche033
move sdf element check
05e739a 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
chapulina
chapulina approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Looks good to me. But since this is going into a released branch, how about we wait for the ign-gazebo integration to be up for review before merging? This way we can be sure we didn't overlook any aspects of the API.

@iche033
Copy link
Contributor Author

iche033 commented Oct 29, 2020

yep sounds good

@iche033 iche033 mentioned this pull request Nov 4, 2020
Merged
@chapulina
Copy link
Contributor

The ign-gazebo PR that uses this works well. It doesn't make use of all elements yet, but I think it's safe to say that it will in the future. So I think this is good to go in.

@chapulina
Copy link
Contributor

@iche033, can we merge this and open some backports as needed?

@iche033 iche033 merged commit d0014d2 into sdf10 Nov 16, 2020
@iche033 iche033 deleted the sky_dom branch November 16, 2020 19:41
iche033 added a commit that referenced this pull request Nov 17, 2020
@iche033 @chapulina
Add Sky DOM (#397)
73cfb9b 
* add sky dom

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* move sdf element check

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
@iche033 iche033 mentioned this pull request Nov 17, 2020
Merged
scpeters added a commit that referenced this pull request Nov 25, 2020
@iche033 @chapulina @scpeters
Add Sky DOM (#397) (#417)
a0d4807 
Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Co-authored-by: Steve Peters <scpeters@openrobotics.org>
azeey pushed a commit to azeey/sdformat that referenced this pull request Dec 9, 2020
@iche033 @chapulina @azeey
Add Sky DOM (gazebosim#397)
5e9c844 
* add sky dom

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* move sdf element check

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
brawner pushed a commit that referenced this pull request Dec 11, 2020
@iche033 @chapulina @brawner
Add Sky DOM (#397)
3363ce7 
* add sky dom

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* move sdf element check

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
brawner pushed a commit that referenced this pull request Jan 9, 2021
@iche033 @chapulina @brawner
Add Sky DOM (#397)
cb0a43c 
* add sky dom

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* feedback changes

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* move sdf element check

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

Co-authored-by: Louise Poubel <louise@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@iche033 @codecov-io @chapulina