Add EnvironmentalData component #1616

hidmic · 2022-07-27T14:21:08Z

🎉 New feature

Summary

This patch adds an EnvironmentalData component that can hold a DataFrame worth of user-defined data for plugins to leverage. Additionally, preload and (interactive) load plugins are included.

Needs gazebosim/gz-common#402.

Test it

Reviewers may try the loader GUI plugin manually.

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
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)

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.

caguero · 2022-08-08T19:25:02Z

src/gui/plugins/environmental_data_loader/EnvironmentalDataLoader.cc

+/// \brief Private data class for EnvironmentalDataLoader
+class EnvironmentalDataLoaderPrivate
+{
+  public: QString dataPath;


Could you document these members?

Done in 16072a7

caguero · 2022-08-08T19:25:20Z

src/gui/plugins/environmental_data_loader/EnvironmentalDataLoader.cc

+}
+}
+
+EnvironmentalDataLoader::EnvironmentalDataLoader()


Nit: Add function delimiters.

Done in 16072a7

caguero · 2022-08-08T21:05:08Z

src/gui/plugins/environmental_data_loader/EnvironmentalDataLoader.cc

+{
+  if (this->dataPtr->needsUpdate)
+  {
+    std::lock_guard<std::mutex> lock(this->dataPtr->mutex);


Should we move this lock before the if above?

@caguero that's the point of using an atomic boolean: avoid unnecessary mutex locking on every simulation step.

Ah, I didn't realize needsLoad was atomic. Then you can move the lock after setting needsLoad to false, right?

@caguero Yes and no. No in the general case, as you could potentially race with another thread to reach the lock. Yes in this particular case as we know there won't more than one thread exercising this execution path (IIRC there's just one thread stepping the simulation).

I went for the generally correct case out of habit. We can move it out of the lock if you want.

chapulina · 2022-08-10T18:16:11Z

Removed the Garden label because we're past feature freeze ❄️ .

This PR can either be merged into main and backported to gz-sim7 after the stable release, or retargeted to gz-sim7 and merged after the stable release (i.e. October).

caguero

Looking good, just waiting for CI to be happy after releasing common.

caguero · 2022-08-16T22:53:03Z

src/gui/plugins/environmental_data_loader/EnvironmentalDataLoader.cc

+{
+  if (this->dataPtr->needsUpdate)
+  {
+    std::lock_guard<std::mutex> lock(this->dataPtr->mutex);


Ah, I didn't realize needsLoad was atomic. Then you can move the lock after setting needsLoad to false, right?

hidmic · 2022-08-18T13:43:55Z

It doesn't look like CI will pass. I'll go ahead and cherry-pick this to our custom branch for the time being. We can come back and merge this once CI stabilizes again.

arjo129 · 2022-08-22T03:19:00Z

src/gui/plugins/environmental_data_loader/EnvironmentalDataLoader.cc

+    try
+    {
+      using ComponentT = components::EnvironmentalData;
+      auto component = ComponentT{common::IO<ComponentT::Type>::ReadFrom(


My understanding is that each world can have only one data frame file. What if two instances are loaded?

What if two instances are loaded?

@arjo129 last component wins, there can only be one component of a given type per entity (and we always target the world entity). See here then here.

arjo129

Just a few clarifications I wanted to ask about.

arjo129 · 2022-09-01T03:37:17Z

include/gz/sim/components/Environment.hh

+    FrameT frame;
+
+    /// \brief Spatial reference for data coordinates. 
+    ReferenceT reference;


I assume CoordinateType::SPHERICAL means we use lat-long. On the other hand LOCAL frame makes no sense here. GLOBAL and ECEF also makes sense, however I don't know if we want to support ECEF right away?

@arjo129 I'll start by saying that I chose to use gz::math::SphericalCoordinates as a reference only because it was convenient. Regarding coordinate system types, what I gather from its implementation is that LOCAL is a rotation away (that specified by heading offsets in spherical coordinates) from GLOBAL. In any case, no code should have to deal with these coordinate systems explicitly. It's just how the data coordinates are expressed.

I assume CoordinateType::SPHERICAL means we use lat-long.

It means the data uses lat-lon coordinates. Code may as well be using global cartesian coordinates. As long as the conversion gets carried out before looking up the frame (see DVL code for an example), it'll be fine. Same for ECEF.

arjo129 · 2022-09-01T03:38:20Z

include/gz/sim/components/Environment.hh

+    using FrameT = common::DataFrame<std::string, T>;
+    using ReferenceT = math::SphericalCoordinates::CoordinateType;
+
+    /// \brief Instantiate environmental data.


Knit: There's a bit of whitespace at EOL issue here. Should be caught when running make codecheck

Done in 36c444d.

arjo129

Sorry as I work through my sensor implementation I spotted something else.

arjo129 · 2022-09-01T07:57:31Z

test/integration/environment_preload_system.cc

@@ -0,0 +1,89 @@
+/*
+ * Copyright (C) 2019 Open Source Robotics Foundation


Knit: License header should be 2022

Done in ebe9d63.

arjo129 · 2022-09-05T03:42:35Z

src/systems/environment_preload/EnvironmentPreload.cc

+  const gz::sim::UpdateInfo &,
+  gz::sim::EntityComponentManager &_ecm)
+{
+  if (!std::exchange(this->dataPtr->loaded, true))


Is there a reason we implement PreUpdate when we probably could get away with doing this in Configure?

@arjo129 it used to be that way. Thing is, we can't query the world SDF (root) path until it's fully loaded, and that has to wait for all world plugins (including this one) to load and configure.

arjo129 · 2022-09-05T13:54:17Z

Oh well then it's fine.

…

On Mon, Sep 5, 2022 at 8:14 PM Michel Hidalgo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/systems/environment_preload/EnvironmentPreload.cc <#1616 (comment)>: > +////////////////////////////////////////////////// +void EnvironmentPreload::Configure( + const Entity &/*_entity*/, + const std::shared_ptr<const sdf::Element> &_sdf, + EntityComponentManager &/*_ecm*/, + EventManager &/*_eventMgr*/) +{ + this->dataPtr->sdf = _sdf; +} + +////////////////////////////////////////////////// +void EnvironmentPreload::PreUpdate( + const gz::sim::UpdateInfo &, + gz::sim::EntityComponentManager &_ecm) +{ + if (!std::exchange(this->dataPtr->loaded, true)) @arjo129 <https://github.com/arjo129> it used to be that way. Thing is, we can't query the world SDF (root) path until it's fully loaded, and that has to wait for all world plugins (including this one) to load and configure. — Reply to this email directly, view it on GitHub <#1616 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEEMQCSUKRQZN5UF4C3QRDV4XPY7ANCNFSM54ZZQFOQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hidmic · 2022-10-17T15:34:52Z

Pending on gazebosim/gz-common#460.

hidmic · 2022-10-17T20:31:22Z

Re-targeting to Garden.

Signed-off-by: Michel Hidalgo <[email protected]>

- Resolve paths relative to SDF parent path - Avoid template instantiation in plugins Signed-off-by: Michel Hidalgo <[email protected]>

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic · 2022-11-14T14:38:47Z

Alright, CI is finally mostly green. Test failures on Windows are unrelated. @arjo129 @caguero may I ask you to do a final review?

caguero

Looks good to me!

Crola1702 · 2022-11-15T15:28:08Z

Hi @hidmic @caguero

We're having a build regression on gz-sim7 debbuilder

Reference build: https://build.osrfoundation.org/job/gz-sim7-debbuilder/184/

Log output:

CMake Warning at /usr/share/cmake-3.16/Modules/CMakeFindDependencyMacro.cmake:47 (find_package):
  By not providing "Findgz-common5-io.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "gz-common5-io", but CMake did not find one.

And then

CMake Error at /usr/share/cmake/gz-cmake3/cmake3/GzConfigureBuild.cmake:73 (message):
  -- BUILD ERRORS: These must be resolved before compiling.
Call Stack (most recent call first):
  CMakeLists.txt:231 (gz_configure_build)


CMake Error at /usr/share/cmake/gz-cmake3/cmake3/GzConfigureBuild.cmake:75 (message):
  -- 	Missing dependency [gz-common5] (Components: profiler, events, av, io)
Call Stack (most recent call first):
  CMakeLists.txt:231 (gz_configure_build)


CMake Error at /usr/share/cmake/gz-cmake3/cmake3/GzConfigureBuild.cmake:77 (message):
  -- END BUILD ERRORS

I think it could be caused by io component added in this PR

hidmic · 2022-11-15T15:29:36Z

@Crola1702 looks like the io component in gz-common5 was not built/released.

nuclearsandwich · 2022-12-02T19:42:01Z

@Crola1702 looks like the io component in gz-common5 was not built/released.

@Crola1702 and I looked at this together, the dependency on libgz-common5-io-dev wasn't added to the debian build dependency data for this package which caused the regression. gazebo-release/gz-sim7-release#18 should resolve that.

hidmic added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Jul 27, 2022

hidmic mentioned this pull request Jul 27, 2022

2022 M3 meta-issue osrf/lrauv#225

Closed

22 tasks

chapulina added enhancement New feature or request 🌱 garden Ignition Garden labels Jul 27, 2022

chapulina added the needs upstream release Blocked by a release of an upstream library label Jul 27, 2022

arjo129 mentioned this pull request Jul 28, 2022

Adds a TimeVaryingVolumetricGrid component [WIP] #1610

Closed

9 tasks

hidmic force-pushed the hidmic/environmental-data branch from 2740c0e to 10a679d Compare August 3, 2022 18:38

hidmic marked this pull request as ready for review August 3, 2022 18:38

hidmic requested a review from chapulina as a code owner August 3, 2022 18:38

hidmic requested a review from caguero August 3, 2022 18:43

chapulina added 🌱 garden Ignition Garden and removed 🌱 garden Ignition Garden labels Aug 6, 2022

caguero reviewed Aug 8, 2022

View reviewed changes

hidmic force-pushed the hidmic/environmental-data branch from 267150c to 16072a7 Compare August 9, 2022 20:15

chapulina removed the 🌱 garden Ignition Garden label Aug 10, 2022

caguero reviewed Aug 16, 2022

View reviewed changes

arjo129 reviewed Aug 22, 2022

View reviewed changes

hidmic mentioned this pull request Aug 29, 2022

Add water-mass tracking support for DVL osrf/lrauv#241

Merged

arjo129 reviewed Sep 1, 2022

View reviewed changes

arjo129 reviewed Sep 5, 2022

View reviewed changes

hidmic changed the base branch from main to gz-sim7 October 17, 2022 20:30

hidmic requested a review from mjcarroll as a code owner October 17, 2022 20:30

nkoenig mentioned this pull request Oct 18, 2022

5.2.0 release gazebosim/gz-common#461

Merged

7 tasks

hidmic added 13 commits November 11, 2022 12:17

Add EnvironmentalDataPreload plugin

f4180de

Signed-off-by: Michel Hidalgo <[email protected]>

Add EnvironmentalDataLoader GUI plugin

de42c6e

Signed-off-by: Michel Hidalgo <[email protected]>

Wrapped up environmental data plugins

c8789b8

Signed-off-by: Michel Hidalgo <[email protected]>

Address review comments

762410c

Signed-off-by: Michel Hidalgo <[email protected]>

Add spatial reference to EnvironmentData

abe1c18

Signed-off-by: Michel Hidalgo <[email protected]>

Fix environmental data preload

e230ef3

- Resolve paths relative to SDF parent path - Avoid template instantiation in plugins Signed-off-by: Michel Hidalgo <[email protected]>

Remove EOL whitespace

3b024af

Signed-off-by: Michel Hidalgo <[email protected]>

Fix license dates

e05ebd2

Signed-off-by: Michel Hidalgo <[email protected]>

Fix EnvironmentLoader QML

a5d2864

Signed-off-by: Michel Hidalgo <[email protected]>

Fix EnvironmentPreload plugin test

4df3424

Signed-off-by: Michel Hidalgo <[email protected]>

Please cpplint

587b6ad

Signed-off-by: Michel Hidalgo <[email protected]>

Make EnvironmentData visible in Windows

1048279

Signed-off-by: Michel Hidalgo <[email protected]>

Disable EnvironmentPreload test on Windows

7f38d57

Signed-off-by: Michel Hidalgo <[email protected]>

hidmic force-pushed the hidmic/environmental-data branch from 9534e89 to 7f38d57 Compare November 11, 2022 22:50

hidmic requested review from caguero and arjo129 and removed request for chapulina November 14, 2022 19:15

caguero approved these changes Nov 14, 2022

View reviewed changes

hidmic merged commit 2d19f93 into gazebosim:gz-sim7 Nov 14, 2022

hidmic deleted the hidmic/environmental-data branch November 14, 2022 23:08

arjo129 mentioned this pull request Nov 17, 2022

Backport #1748: Adds a tool for environment data visualization and custom environmental sensors #1798

Merged

9 tasks

nuclearsandwich mentioned this pull request Dec 2, 2022

Add build dependency on libgz-common5-io-dev. gazebo-release/gz-sim7-release#18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add EnvironmentalData component #1616

Add EnvironmentalData component #1616

hidmic commented Jul 27, 2022 •

edited

Loading

caguero Aug 8, 2022

hidmic Aug 9, 2022 •

edited

Loading

caguero Aug 8, 2022

hidmic Aug 9, 2022 •

edited

Loading

caguero Aug 8, 2022

hidmic Aug 9, 2022

caguero Aug 16, 2022

hidmic Aug 17, 2022

chapulina commented Aug 10, 2022

caguero left a comment

caguero Aug 16, 2022

hidmic commented Aug 18, 2022

arjo129 Aug 22, 2022 •

edited

Loading

hidmic Aug 22, 2022

arjo129 left a comment

arjo129 Sep 1, 2022

hidmic Sep 5, 2022 •

edited

Loading

arjo129 Sep 1, 2022

hidmic Sep 12, 2022

arjo129 left a comment •

edited

Loading

arjo129 Sep 1, 2022

hidmic Sep 12, 2022

arjo129 Sep 5, 2022

hidmic Sep 5, 2022

arjo129 commented Sep 5, 2022 via email

hidmic commented Oct 17, 2022

hidmic commented Oct 17, 2022

hidmic commented Nov 14, 2022

caguero left a comment

Crola1702 commented Nov 15, 2022

hidmic commented Nov 15, 2022

nuclearsandwich commented Dec 2, 2022

		@@ -0,0 +1,89 @@
		/*
		* Copyright (C) 2019 Open Source Robotics Foundation

Add EnvironmentalData component #1616

Add EnvironmentalData component #1616

Conversation

hidmic commented Jul 27, 2022 • edited Loading

🎉 New feature

Summary

Test it

Checklist

Choose a reason for hiding this comment

hidmic Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Aug 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chapulina commented Aug 10, 2022

caguero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Aug 18, 2022

arjo129 Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjo129 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjo129 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arjo129 commented Sep 5, 2022 via email

hidmic commented Oct 17, 2022

hidmic commented Oct 17, 2022

hidmic commented Nov 14, 2022

caguero left a comment

Choose a reason for hiding this comment

Crola1702 commented Nov 15, 2022

hidmic commented Nov 15, 2022

nuclearsandwich commented Dec 2, 2022

hidmic commented Jul 27, 2022 •

edited

Loading

hidmic Aug 9, 2022 •

edited

Loading

hidmic Aug 9, 2022 •

edited

Loading

arjo129 Aug 22, 2022 •

edited

Loading

hidmic Sep 5, 2022 •

edited

Loading

arjo129 left a comment •

edited

Loading