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

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 [email protected]

Signed-off-by: Ian Chen <[email protected]>
@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

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.

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
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

@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
* add sky dom

Signed-off-by: Ian Chen <[email protected]>

* feedback changes

Signed-off-by: Ian Chen <[email protected]>

* move sdf element check

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
@iche033 iche033 mentioned this pull request Nov 17, 2020
scpeters added a commit that referenced this pull request Nov 25, 2020
Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
azeey pushed a commit to azeey/sdformat that referenced this pull request Dec 9, 2020
* add sky dom

Signed-off-by: Ian Chen <[email protected]>

* feedback changes

Signed-off-by: Ian Chen <[email protected]>

* move sdf element check

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
brawner pushed a commit that referenced this pull request Dec 11, 2020
* add sky dom

Signed-off-by: Ian Chen <[email protected]>

* feedback changes

Signed-off-by: Ian Chen <[email protected]>

* move sdf element check

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
brawner pushed a commit that referenced this pull request Jan 9, 2021
* add sky dom

Signed-off-by: Ian Chen <[email protected]>

* feedback changes

Signed-off-by: Ian Chen <[email protected]>

* move sdf element check

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants