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

Minor tweaks to visual cloning #418

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Sep 21, 2021

Signed-off-by: Ian Chen [email protected]

Made a few minor tweaks to API and documentation

Signed-off-by: Ian Chen <[email protected]>
@iche033 iche033 requested a review from adlarkin September 21, 2021 22:22
@iche033 iche033 mentioned this pull request Sep 21, 2021
8 tasks
Copy link
Contributor

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question before merging

this->meshDescriptor.meshName = _copy.meshName;
this->meshDescriptor.subMeshName = _copy.subMeshName;
this->meshDescriptor.centerSubMesh = _copy.centerSubMesh;
this->meshDescriptor = _desc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this also copy the pointer to the mesh? As we discussed offline, I thought that we wanted to ignore storing/keeping track of this pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I took another look at the implementation and looks like the common::Mesh pointer is managed by the common::MeshManager and the pointer remains valid for the duration of the simulation session so it maybe ok. I don't have a strong opinion on this though

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the pointer is valid, I am fine with storing it. Since it's a pointer, it doesn't take up much memory anyways. I say go ahead and merge it!

@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #418 (2610bc7) into adlarkin/clone_visual (42f0484) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

❗ Current head 2610bc7 differs from pull request most recent head 2f6d7d3. Consider uploading reports for the commit 2f6d7d3 to get more accurate results
Impacted file tree graph

@@                    Coverage Diff                    @@
##           adlarkin/clone_visual     #418      +/-   ##
=========================================================
- Coverage                  55.19%   55.18%   -0.01%     
=========================================================
  Files                        191      191              
  Lines                      19351    19348       -3     
=========================================================
- Hits                       10681    10678       -3     
  Misses                      8670     8670              
Impacted Files Coverage Δ
include/ignition/rendering/Mesh.hh 100.00% <ø> (ø)
ogre/src/OgreMeshFactory.cc 1.03% <ø> (+<0.01%) ⬆️
ogre/src/OgreScene.cc 26.57% <0.00%> (-0.09%) ⬇️
ogre2/src/Ogre2MeshFactory.cc 78.91% <ø> (-0.08%) ⬇️
include/ignition/rendering/base/BaseMesh.hh 65.57% <100.00%> (-0.83%) ⬇️
ogre2/src/Ogre2RenderEngine.cc 79.80% <100.00%> (ø)
ogre2/src/Ogre2Scene.cc 76.73% <100.00%> (+0.03%) ⬆️

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 42f0484...2f6d7d3. Read the comment docs.

@chapulina chapulina added beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress labels Sep 22, 2021
@iche033 iche033 merged commit 6791b41 into adlarkin/clone_visual Sep 22, 2021
@iche033 iche033 deleted the iche033/clone_visual branch September 22, 2021 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants