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

Merge ign-gazebo6 ➡️ main (part 2) #1631

Merged
merged 13 commits into from
Aug 9, 2022
Merged

Merge ign-gazebo6 ➡️ main (part 2) #1631

merged 13 commits into from
Aug 9, 2022

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Aug 5, 2022

➡️ Forward port

Port ign-gazebo6 ➡️ main

Branch comparison: main...ign-gazebo6

@mjcarroll , I could use some help getting the changes from #1352 to play nicely with the reset stuff - #1390 is a nice reference.

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

chapulina and others added 12 commits June 22, 2022 19:29
Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Michael Carroll <[email protected]>
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Carlos Agüero <[email protected]>
Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Ian Chen <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Jorge Perez <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina requested review from iche033 and azeey as code owners August 5, 2022 02:39
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 5, 2022
@chapulina chapulina changed the base branch from main to chapulina/6_to_7_pt1 August 5, 2022 02:40
@chapulina chapulina requested a review from mjcarroll August 5, 2022 02:41
@chapulina chapulina changed the title Merge ign-gazebo6 ➡️ main Merge ign-gazebo6 ➡️ main (part 2) Aug 5, 2022
Base automatically changed from chapulina/6_to_7_pt1 to main August 5, 2022 16:15
chapulina added a commit that referenced this pull request Aug 5, 2022
chapulina added a commit that referenced this pull request Aug 5, 2022
@chapulina chapulina force-pushed the chapulina/6_to_7_pt2 branch from ba879d3 to d5c08b2 Compare August 5, 2022 17:03
chapulina added a commit that referenced this pull request Aug 5, 2022
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina force-pushed the chapulina/6_to_7_pt2 branch from d5c08b2 to ed5cd0b Compare August 5, 2022 18:57
chapulina added a commit that referenced this pull request Aug 5, 2022
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina force-pushed the chapulina/6_to_7_pt2 branch from ed5cd0b to 5e0847d Compare August 5, 2022 19:01
@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #1631 (f8deb17) into main (a3cbd8c) will decrease coverage by 0.15%.
The diff coverage is 99.21%.

❗ Current head f8deb17 differs from pull request most recent head ee48126. Consider uploading reports for the commit ee48126 to get more accurate results

@@            Coverage Diff             @@
##             main    #1631      +/-   ##
==========================================
- Coverage   63.62%   63.47%   -0.16%     
==========================================
  Files         330      331       +1     
  Lines       25787    26123     +336     
==========================================
+ Hits        16408    16581     +173     
- Misses       9379     9542     +163     
Impacted Files Coverage Δ
include/gz/sim/EventManager.hh 79.16% <ø> (ø)
include/gz/sim/Util.hh 100.00% <ø> (ø)
include/gz/sim/components/Serialization.hh 95.55% <ø> (ø)
include/gz/sim/components/Factory.hh 97.53% <80.00%> (-1.18%) ⬇️
include/gz/sim/EntityComponentManager.hh 100.00% <100.00%> (ø)
include/gz/sim/Link.hh 100.00% <100.00%> (ø)
include/gz/sim/Model.hh 100.00% <100.00%> (ø)
include/gz/sim/SdfEntityCreator.hh 100.00% <100.00%> (ø)
include/gz/sim/Server.hh 100.00% <100.00%> (ø)
include/gz/sim/ServerConfig.hh 100.00% <100.00%> (ø)
... and 133 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

chapulina added a commit that referenced this pull request Aug 6, 2022
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina force-pushed the chapulina/6_to_7_pt2 branch from 5e0847d to f8deb17 Compare August 6, 2022 01:57
chapulina added a commit that referenced this pull request Aug 8, 2022
Signed-off-by: Louise Poubel <[email protected]>
@chapulina chapulina force-pushed the chapulina/6_to_7_pt2 branch from f8deb17 to ede7141 Compare August 8, 2022 16:20
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor Author

CI result analysis:

@mjcarroll
Copy link
Contributor

I have on my list for this week:

  • INTEGRATION_scene_broadcaster_system - I have most of a fix for this, just working out a last timing kink on all platforms.
  • Windows: No failures, just pre-existing warnings - I have spun up a brand new Windows install to work through windows test failures and warnings throughout the stack.

@mjcarroll
Copy link
Contributor

I have tested the reset examples locally and this looks like a clean merge, functionality is intact. I don't see any weird interactions here.

@chapulina
Copy link
Contributor Author

I have tested the reset examples locally and this looks like a clean merge, functionality is intact. I don't see any weird interactions here.

Thanks, @mjcarroll . I'll consider this an approval and merge this PR, we're way past feature freeze and need to branch off gz-sim7 asap.

@chapulina chapulina merged commit 3f57c77 into main Aug 9, 2022
@chapulina chapulina deleted the chapulina/6_to_7_pt2 branch August 9, 2022 15:09
Copy link
Contributor

@jennuine jennuine left a comment

Choose a reason for hiding this comment

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

I found some ign console loggers that should be updated. Should I open a separate PR for these?

{
if (_plugin.Filename() == "" || _plugin.Name() == "")
{
ignerr << "Failed to instantiate system plugin: empty argument "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignerr << "Failed to instantiate system plugin: empty argument "
gzerr << "Failed to instantiate system plugin: empty argument "

std::string entitySystemService{"entity/system/add"};
this->node->Advertise(entitySystemService,
&SystemManager::EntitySystemAddService, this);
ignmsg << "Serving entity system service on ["
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignmsg << "Serving entity system service on ["
gzmsg << "Serving entity system service on ["


if (req.plugins().empty())
{
ignwarn << "Unable to add plugins to Entity: '" << entity
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignwarn << "Unable to add plugins to Entity: '" << entity
gzwarn << "Unable to add plugins to Entity: '" << entity

@@ -427,15 +427,15 @@ void addResourcePaths(const std::vector<std::string> &_paths)
char *ignPathCStr = std::getenv(systemPaths->FilePathEnv().c_str());
if (ignPathCStr && *ignPathCStr != '\0')
{
ignPaths = common::Split(ignPathCStr, ':');
ignPaths = common::Split(ignPathCStr, common::SystemPaths::Delimiter());
Copy link
Contributor

Choose a reason for hiding this comment

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

Does ign need to be replaced with gz here? If not, we should update the above comment to say Ignition file paths... with a Deprecated note

animMesh = meshManager->Load(animFilename);
if (animMesh->MeshSkeleton()->AnimationCount() > 1)
{
ignwarn << "File [" << animFilename
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignwarn << "File [" << animFilename
gzwarn << "File [" << animFilename

auto firstAnim = animMesh->MeshSkeleton()->Animation(0);
if (nullptr == firstAnim)
{
ignerr << "Failed to get animations from [" << animFilename
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ignerr << "Failed to get animations from [" << animFilename
gzerr << "Failed to get animations from [" << animFilename

Comment on lines +2378 to +2379
// add a copy constructor to animation classes in ign-common.
// The proper fix is probably to update ign-rendering to allow it to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// add a copy constructor to animation classes in ign-common.
// The proper fix is probably to update ign-rendering to allow it to
// add a copy constructor to animation classes in gz-common.
// The proper fix is probably to update gz-rendering to allow it to

@chapulina
Copy link
Contributor Author

I found some ign console loggers that should be updated. Should I open a separate PR for these?

Thanks, @jennuine ! Those are taken care of in #1633

@j-rivero j-rivero mentioned this pull request Sep 16, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants