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

Support 'has' functionality in particle Emitter. #137

Merged
merged 3 commits into from
Feb 26, 2021

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Feb 25, 2021

Signed-off-by: Nate Koenig [email protected]

🦟 Bug fix

Summary

The particle emitter message used plain data types. This prevents an Ignition system from knowing if a value has been set. Since we are using the emitter message as the Component in Ignition, we need this information.

I'm changing the message types. Since this message was just introduced the impact should be minimal.

Required by: gazebosim/gz-sim#651

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: Nate Koenig <[email protected]>
@nkoenig nkoenig requested a review from iche033 February 25, 2021 05:37
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Feb 25, 2021
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #137 (528b937) into ign-msgs6 (185d467) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-msgs6     #137   +/-   ##
==========================================
  Coverage      84.59%   84.59%           
==========================================
  Files              7        7           
  Lines            818      818           
==========================================
  Hits             692      692           
  Misses           126      126           

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 185d467...528b937. Read the comment docs.

@nkoenig nkoenig changed the title Support functionality. Support 'has' functionality in particle Emitter. Feb 25, 2021
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.

This breaks API and can't go into a stable release. It needs to be retargeted to main, and even then, I think we should consider a tick-tock strategy.

@nkoenig
Copy link
Contributor Author

nkoenig commented Feb 25, 2021

This breaks API and can't go into a stable release. It needs to be retargeted to main, and even then, I think we should consider a tick-tock strategy.

The particle emitter was added in the last minor release. I'm making this change in a subsequent minor release. The impact is minimal and is needed by subt.

@chapulina
Copy link
Contributor

Ok, let's make this exception since there are probably no users of particles yet and the release is still fresh. In the future, we should consider making pre-releases for new features that are still under development.

We need to time well the next ign-msgs6and ign-gazebo4 releases so that no users are left in a broken state, and it's very important to set minimum required versions on both ign-gazebo and ign-gazebo4-release.

Can you add a note about the ABI break to the migration guide?

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor Author

nkoenig commented Feb 26, 2021

Can you add a note about the ABI break to the migration guide?

Added in ef3e3f4

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig nkoenig merged commit e270983 into ign-msgs6 Feb 26, 2021
@nkoenig nkoenig deleted the particle_modification branch February 26, 2021 18:58
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