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

Fix component updates #1580

Merged
merged 7 commits into from
Jul 20, 2022
Merged

Fix component updates #1580

merged 7 commits into from
Jul 20, 2022

Conversation

jennuine
Copy link
Contributor

@jennuine jennuine commented Jul 7, 2022

🦟 Bug fix

Fixes #1022

Summary

To speed up performance, removed components were cached/stored here:

/// The reason for moving entities with missing components to invalidData
/// instead of completely deleting them from the view is because if components
/// are added back later and the entity needs to be re-added to the view,
/// tuple creation can be costly. So, this approach is used instead to
/// maintain runtime performance (the tradeoff of mainting performance is
/// increased complexity and memory usage).
///
/// \sa missingCompTracker
private: std::unordered_map<Entity, ComponentData> invalidData;
private: std::unordered_map<Entity, ConstComponentData> invalidConstData;

Then when the same component was re-added:

viewPair.second.first->NotifyComponentAddition(_entity,
this->IsNewEntity(_entity), _componentTypeId);

The component data was moved from invalidData to validData but the data was never updated.

The fix is to check that component needs updated in the EntityComponentManager::SetState functions then retrieve the component and deserialize the data again (to update the re-added component).


To run unit tests:

./path/to/build/UNIT_EntityComponentManager_TEST --gtest_filter="*AddRemoveAddComponents*"

To test through the GUI:

ign gazebo -v 4 lights.sdf

Before PR
before_pr

After PR
after_pr

Thanks @chapulina for the help!

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.

@jennuine jennuine requested a review from chapulina as a code owner July 7, 2022 16:47
@jennuine jennuine self-assigned this Jul 7, 2022
@jennuine jennuine added bug Something isn't working 🏯 fortress Ignition Fortress labels Jul 7, 2022
@chapulina chapulina added the OOBE 📦✨ Out-of-box experience label Jul 7, 2022
@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #1580 (bb7f37e) into ign-gazebo6 (d6e69d0) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           ign-gazebo6    #1580      +/-   ##
===============================================
+ Coverage        64.02%   64.14%   +0.12%     
===============================================
  Files              317      317              
  Lines            25649    25667      +18     
===============================================
+ Hits             16422    16465      +43     
+ Misses            9227     9202      -25     
Impacted Files Coverage Δ
src/EntityComponentManager.cc 89.35% <100.00%> (-0.01%) ⬇️
src/Util.cc 92.53% <100.00%> (ø)
src/systems/user_commands/UserCommands.cc 57.67% <100.00%> (+4.34%) ⬆️
src/SimulationRunner.cc 90.93% <0.00%> (-0.96%) ⬇️

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 f39bc25...bb7f37e. Read the comment docs.

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I can confirm that this is working like a charm! Thank you!

I had a minor comment about the error message

src/EntityComponentManager.cc Outdated Show resolved Hide resolved
@jennuine jennuine merged commit 95f3820 into ign-gazebo6 Jul 20, 2022
@jennuine jennuine deleted the jennuine/fix_component_updates branch July 20, 2022 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏯 fortress Ignition Fortress OOBE 📦✨ Out-of-box experience
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Modifying lights in component inspector not working
3 participants