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

Adds a validity check for Sessions created using the TimeVaryingVolumetricGrid #551

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Aug 28, 2023

🦟 Bug fix

Fixes #

Summary

This looks like a feature... and it is but it is needed to resolve a bug in gazebosim/gz-sim#1842 (review). In particular part of the issue is poor API design. Ideally CreateSession would return an Option<Session>, however it returns a Session even if it is not possible to create the session (for instance if it is too far ahead in time or an empty grid). Hence downstream users need a way to verify session validity.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

`TimeVaryingVolumetricGrid`

This is needed to resolve a bug in gazebosim/gz-sim#1842 (review). In particular part of the issue is poor API design. Ideally `CreateSession` would return an `Option<Session>`, however it returns a `Session` even if it is not possible to create the session (for instance if it is too far ahead in time or an empty grid). Hence downstream users need a way to verify session validity.

Signed-off-by: Arjo Chakravarty <[email protected]>
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Aug 28, 2023
arjo129 added a commit to gazebosim/gz-sim that referenced this pull request Aug 28, 2023
However fix depends on gazebosim/gz-math#551

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #551 (446916b) into gz-math7 (d32291d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 446916b differs from pull request most recent head 63abc22. Consider uploading reports for the commit 63abc22 to get more accurate results

@@            Coverage Diff            @@
##           gz-math7     #551   +/-   ##
=========================================
  Coverage     94.16%   94.16%           
=========================================
  Files           145      145           
  Lines          9768     9774    +6     
=========================================
+ Hits           9198     9204    +6     
  Misses          570      570           
Files Changed Coverage Δ
include/gz/math/TimeVaryingVolumetricGrid.hh 100.00% <100.00%> (ø)
...de/gz/math/TimeVaryingVolumetricGridLookupField.hh 100.00% <100.00%> (ø)

Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

just one minor comment about removing commented out code, otherwise looks good

Signed-off-by: Arjo Chakravarty <[email protected]>
@azeey azeey merged commit 3d9cf33 into gz-math7 Aug 29, 2023
8 checks passed
@azeey azeey deleted the arjo/fix/add_an_invalid_session_check branch August 29, 2023 15:23
arjo129 added a commit to gazebosim/gz-sim that referenced this pull request Oct 3, 2023
* Fix enviroment system loading mechanism

Currently, there is an issue with the way the Environment loader plugin loads data. In particular it directly writes to the ECM. While this makes sense intuitively, it does not work in practice as the GUI runs on a client process while systems that use it run on the server. This PR fixes this issue by introducing a topic through which the GUI may load Environment Data on the server.

Signed-off-by: Arjo Chakravarty <[email protected]>

* small changes

Signed-off-by: Arjo Chakravarty <[email protected]>

* Working on porting the visuals

Signed-off-by: Arjo Chakravarty <[email protected]>

* Actually send message for loading from ui to environment preload plugin.

Visuallization still goes 💥

Signed-off-by: Arjo Chakravarty <[email protected]>

* Rewrite EnvironmentVisualization Widget to be simpler.

Signed-off-by: Arjo Chakravarty <[email protected]>

* fix crashes.

Vis still not working

Signed-off-by: Arjo Chakravarty <[email protected]>

* Get a different 💥

Signed-off-by: Arjo Chakravarty <[email protected]>

* Works some times.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fixed synchronization issues.

Now left with one more crash that needs debugging when "play" is hit.

Signed-off-by: Arjo Chakravarty <[email protected]>

* No more 💥s 🎉

Signed-off-by: Arjo Chakravarty <[email protected]>

* style

Signed-off-by: Arjo Chakravarty <[email protected]>

* Sprinkled with healthy dose of Doxygen

Also refactored the visualization tool out.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Style

Signed-off-by: Arjo Chakravarty <[email protected]>

* More style fixes

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fix Typo with unit map

Signed-off-by: Arjo Chakravarty <[email protected]>

* Address PR feedback

Signed-off-by: Arjo Chakravarty <[email protected]>

* Style fixes

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fix incorrect use of path.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fix example loading issues.

Signed-off-by: Arjo Chakravarty <[email protected]>

* style

Signed-off-by: Arjo Chakravarty <[email protected]>

* Update src/systems/environment_preload/VisualizationTool.cc

Co-authored-by: Mabel Zhang <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>

* Adds a warning regarding loading plugins.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Automatically loads plugin if missing

This commit automatically loads the environment preload plugin if it is
missing.

Signed-off-by: Arjo Chakravarty <[email protected]>

* Address some feedback I missed

Signed-off-by: Arjo Chakravarty <[email protected]>

* Address some feedback

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fixes issue  described by @iche033.

However fix depends on gazebosim/gz-math#551

Signed-off-by: Arjo Chakravarty <[email protected]>

* style

Signed-off-by: Arjo Chakravarty <[email protected]>

* Fixed failing tests

Signed-off-by: Arjo Chakravarty <[email protected]>

---------

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Mabel Zhang <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants