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

Refactor RenderUtil::Update with helper functions #858

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Jun 15, 2021

Summary

#853 (comment) makes changes that result in RenderUtil::Update being longer than 500 lines of code (yikes!). It looks like several new features were introduced in ign-gazebo4 (thermal camera, lights, etc.) that have added a lot of code to RenderUtil::Update. So, I have introduced some helper functions that can be called in RenderUtil::Update to reduce this method's size.

Once this is merged, we can forward-port this so that all other branches/PRs don't have this issue appear anymore. It's also worth noting that I only introduced helper functions for code that was introduced in ign-gazebo4 so that we (hopefully) don't get any nasty merge conflicts when we merge ign-gazebo3 forward.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • 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

Signed-off-by: Ashton Larkin <ashton@openrobotics.org>
@adlarkin adlarkin requested a review from iche033 as a code owner June 15, 2021 19:28
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jun 15, 2021
Copy link
Contributor Author

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

I've got a few comments/questions for PR reviewers.

src/rendering/RenderUtil.cc Show resolved Hide resolved
src/rendering/RenderUtil.cc Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #858 (2964109) into ign-gazebo4 (24d8949) will decrease coverage by 0.00%.
The diff coverage is 42.22%.

❗ Current head 2964109 differs from pull request most recent head d60121c. Consider uploading reports for the commit d60121c to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4     #858      +/-   ##
===============================================
- Coverage        65.69%   65.68%   -0.01%     
===============================================
  Files              240      240              
  Lines            17797    17811      +14     
===============================================
+ Hits             11691    11700       +9     
- Misses            6106     6111       +5     
Impacted Files Coverage Δ
src/rendering/RenderUtil.cc 42.12% <42.22%> (+0.33%) ⬆️

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 24d8949...d60121c. Read the comment docs.

@adlarkin adlarkin mentioned this pull request Jun 16, 2021
8 tasks
@atharva-18 atharva-18 mentioned this pull request Jun 16, 2021
8 tasks
src/rendering/RenderUtil.cc Outdated Show resolved Hide resolved
src/rendering/RenderUtil.cc Show resolved Hide resolved
src/rendering/RenderUtil.cc Show resolved Hide resolved
@adlarkin adlarkin requested a review from iche033 June 18, 2021 10:57
@adlarkin adlarkin merged commit a1ebffa into ign-gazebo4 Jun 18, 2021
@adlarkin adlarkin deleted the adlarkin/renderutil_update_refactor branch June 18, 2021 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants