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 scatter ratio parameter to Particle Emitter DOM #547

Merged
merged 4 commits into from
May 6, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Apr 26, 2021

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

🎉 New feature

Summary

Add particle scatter parameter to the Particle Emitter DOM. This affects how noisy / scattered the sensors readings are affected by the particle emitter.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • 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: Ian Chen <ichen@osrfoundation.org>
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Apr 26, 2021
@iche033 iche033 changed the base branch from sdf11 to sdf10 April 26, 2021 22:17
@iche033 iche033 added 🔮 dome Ignition Dome and removed 🏢 edifice Ignition Edifice labels Apr 26, 2021
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2021

Codecov Report

Merging #547 (2256147) into sdf10 (f310d75) will increase coverage by 0.00%.
The diff coverage is 87.50%.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf10     #547   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files          63       63           
  Lines        9880     9888    +8     
=======================================
+ Hits         8622     8629    +7     
- Misses       1258     1259    +1     
Impacted Files Coverage Δ
src/ParticleEmitter.cc 76.92% <87.50%> (+0.39%) ⬆️

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 f310d75...2256147. Read the comment docs.

Copy link
Contributor

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

Just a few minor comments

include/sdf/ParticleEmitter.hh Outdated Show resolved Hide resolved
sdf/1.7/particle_emitter.sdf Outdated Show resolved Hide resolved
src/ParticleEmitter.cc Outdated Show resolved Hide resolved
/////////////////////////////////////////////////
void ParticleEmitter::SetScatterRatio(float _ratio)
{
this->dataPtr->scatterRatio = _ratio;
Copy link
Contributor

@adlarkin adlarkin May 5, 2021

Choose a reason for hiding this comment

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

Suggested change
this->dataPtr->scatterRatio = _ratio;
if (_ratio <= 0.0f)
{
ignwarn << "Scatter ratio must be > 0, but a value of " << _ratio << " was given. Using a value of " << this->dataPtr->scatterRatio << " instead" << std::endl;
return;
}
this->dataPtr->scatterRatio = _ratio;

Since the scatter ratio must be > 0, should we only set it if the value passed in is > 0? If so, we should also update the documentation to indicate this in ParticleEmitter.hh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after thinking about it more, I removed the requirement on the value needs to be > 0. It's more of an implementation limitation on the ignition end. Since we should keep the spec as general as possible, I think someone could decide to use 0 to simulate 0 scattering for certain sensors.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. Everything looks good to me, so feel free to merge unless you're waiting for someone else to look over this as well 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, will merge after CI builds are done

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
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.

None yet

3 participants