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.

Sign up for GitHub

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

Jump to bottom

Added capsule geometry #200

Merged
merged 29 commits into from
Mar 20, 2021
Merged

Added capsule geometry #200

merged 29 commits into from
Mar 20, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jan 19, 2021

Added capsule geometry. Related with this issue #197

  • Ogre
    capsule_ogre

  • Ogre2
    capsule_ogre2

Signed-off-by: ahcorde [email protected]

@ahcorde
Added capsule geometry
07742ff 
Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde added the 🏰 citadel Ignition Citadel label Jan 19, 2021
@ahcorde ahcorde requested a review from iche033 January 19, 2021 11:19
@ahcorde ahcorde self-assigned this Jan 19, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jan 19, 2021
@ahcorde
make linters happy
d66ee33 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
make linters happy
e30745f 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
quick fix
720a178 
Signed-off-by: ahcorde <[email protected]>
@codecov
Copy link

codecov bot commented Jan 19, 2021 •
edited
Loading

Codecov Report

Merging #200 (223ed97) into main (3584189) will decrease coverage by 0.23%.
The diff coverage is 37.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
- Coverage   57.77%   57.54%   -0.24%     
==========================================
  Files         153      159       +6     
  Lines       15411    15595     +184     
==========================================
+ Hits         8904     8974      +70     
- Misses       6507     6621     +114
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
...gre/include/ignition/rendering/ogre/OgreCapsule.hh 0.00% <0.00%> (ø)
ogre/src/OgreCapsule.cc 0.00% <0.00%> (ø)
ogre/src/OgreMarker.cc 0.00% <0.00%> (ø)
ogre/src/OgreScene.cc 28.07% <0.00%> (-0.40%) ⬇️
ogre2/src/Ogre2Marker.cc 46.09% <45.65%> (-2.62%) ⬇️
ogre2/src/Ogre2Capsule.cc 61.01% <61.01%> (ø)
include/ignition/rendering/Capsule.hh 100.00% <100.00%> (ø)
include/ignition/rendering/base/BaseCapsule.hh 100.00% <100.00%> (ø)
... and 10 more

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 3584189...223ed97. Read the comment docs.

@ahcorde ahcorde removed the 🏰 citadel Ignition Citadel label Jan 20, 2021
@ahcorde
Fixed typo
02396d6 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Added Capsule_TEST
7b952d3 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Fixed test
574f34c 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Updated capsule
fa1bd16 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 21, 2021

Depends on gazebosim/gz-common#155

@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jan 21, 2021
@ahcorde ahcorde mentioned this pull request Jan 25, 2021
Merged
@ahcorde
updated code
7055466 
Signed-off-by: ahcorde <[email protected]>
@chapulina
Copy link
Contributor

Same question as for the ellipsoid. Do we need a Capsule class? Maybe this is better, I'm just trying to understanding the rationale.

@ahcorde ahcorde mentioned this pull request Feb 4, 2021
Closed
@ahcorde ahcorde linked an issue Feb 4, 2021 that may be closed by this pull request
Support capsule geometry #197
Closed
@ahcorde ahcorde mentioned this pull request Feb 4, 2021
Closed
chapulina
chapulina suggested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Do we need a Capsule class?

Sorry for backtracking, but after the conversation in gazebosim/gz-common#165, I think the capsule will need special treatment here.

Unlike the other simple shapes (box, sphere, cylinder, ellipsoid, or even cone in the future), a capsule can't be scaled to an arbitrary length + radius through the SetLocalScale function. Instead, ign-rendering will need to offer API that lets users change those 2 dimensions specifically.

@ahcorde ahcorde force-pushed the ahcorde/geom/capsule branch from ffd8d66 to 7055466 Compare February 12, 2021 07:18
@ahcorde ahcorde force-pushed the ahcorde/geom/capsule branch from 5f5d699 to 97b5bf7 Compare March 18, 2021 12:35
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 18, 2021

I'm getting this error

simple_demo: /var/lib/jenkins/workspace/ogre-2.1-debbuilder/repo/OgreMain/src/OgreHlmsDatablock.cpp:170: virtual Ogre::HlmsDatablock::~HlmsDatablock(): Assertion `mLinkedRenderables.empty() && "This Datablock is still being used by some Renderables." " Change their Datablocks before destroying this."' failed.
Abortado (`core' generado)

I'm not really sure why

ahcorde added 5 commits March 18, 2021 14:22
@ahcorde
Fix capsule material issue
09d7b20 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Fixed Ogre2Marker
e7e4597 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
make linters happy
e8c9d06 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
make linters happy
decd32e 
Signed-off-by: ahcorde <[email protected]>
@ahcorde
Removed macos warning
d779159 
Signed-off-by: ahcorde <[email protected]>
@iche033
Copy link
Contributor

iche033 commented Mar 19, 2021

I'm getting this error

When do you get that error?

iche033
iche033 reviewed Mar 19, 2021
View reviewed changes
include/ignition/rendering/Capsule.hh Outdated Show resolved Hide resolved
examples/simple_demo/Main.cc Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 19, 2021

When do you get that error?

I fixed the error in one of the commits

@ahcorde ahcorde requested review from iche033 and chapulina March 19, 2021 08:49
@ahcorde ahcorde mentioned this pull request Mar 19, 2021
Merged
@iche033
fixing windows warnings
83681f6 
Signed-off-by: Ian Chen <[email protected]>
chapulina added a commit that referenced this pull request Mar 19, 2021
@chapulina
Suggestions to capsule (#200)
a7e6bc1 
Signed-off-by: Louise Poubel <[email protected]>
chapulina
chapulina suggested changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I have some suggestions for this PR on #276

ogre2/src/Ogre2Capsule.cc Outdated Show resolved Hide resolved
@chapulina
Suggestions to #200 (#276)
223ed97 
Signed-off-by: Louise Poubel <[email protected]>
chapulina
chapulina approved these changes Mar 20, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

💊 🎉

@chapulina chapulina merged commit eefcdd9 into main Mar 20, 2021
@chapulina chapulina deleted the ahcorde/geom/capsule branch March 20, 2021 04:38
This was referenced Jun 5, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@iche033 iche033 Awaiting requested review from iche033 iche033 is a code owner

Assignees

@ahcorde ahcorde

Labels
beta Targeting beta release of upcoming collection 🏢 edifice Ignition Edifice
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support capsule geometry
3 participants
@ahcorde @chapulina @iche033