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

Add laser_retro in Visual #454

Merged
merged 13 commits into from
Jan 25, 2021
Merged

Add laser_retro in Visual #454

merged 13 commits into from
Jan 25, 2021

Conversation

doisyg
Copy link
Contributor

@doisyg doisyg commented Jan 6, 2021

First PR needed for laser_retro support as discussed here: gazebosim/gz-rendering#181

@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 6, 2021
@doisyg doisyg mentioned this pull request Jan 6, 2021
Closed
@codecov-io
Copy link

codecov-io commented Jan 6, 2021 •
edited
Loading

Codecov Report

Merging #454 (19a8587) into sdf9 (0989bff) will decrease coverage by 0.01%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             sdf9     #454      +/-   ##
==========================================
- Coverage   86.68%   86.67%   -0.02%     
==========================================
  Files          61       61              
  Lines        9516     9531      +15     
==========================================
+ Hits         8249     8261      +12     
- Misses       1267     1270       +3
Impacted Files Coverage Δ
src/Visual.cc 90.44% <81.25%> (-1.30%) ⬇️

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 0989bff...2b0d37f. Read the comment docs.

@chapulina chapulina mentioned this pull request Jan 6, 2021
Merged
chapulina
chapulina reviewed Jan 6, 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.

Thanks for the PR. Mind adding some tests for the collision too? And also signing the commits for DCO?

include/sdf/Collision.hh Outdated

/// \brief Set the lidar reflective intensity.
/// \param[in] _laserRetro The lidar reflective intensity.
public: void SetLaserRetro(double _laserRetro);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind moving the new public functions up with the existing ones?

include/sdf/Collision.hh Outdated
/// has been specified.
/// \param[in] _laserRetro True if the lidar reflective intensity
/// has been set in the sdf.
public: void SetHasLaserRetro(bool _laserRetro);
Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't been consistently adding Has functions for optional elements. I only see this in Camera.hh. I'm not sure if that's the direction we want to go in, or if we want to just leave the default value in case it's unset.

Thoughts, @azeey ?

src/Visual.cc Outdated Show resolved Hide resolved
Guillaume Doisy and others added 7 commits January 6, 2021 20:13
add load
0494ea1 
Signed-off-by: Guillaume Doisy <[email protected]>
add Has and Set functions
311f3bd 
Signed-off-by: Guillaume Doisy <[email protected]>
add visual laser retro + tests
3573cb1 
Signed-off-by: Guillaume Doisy <[email protected]>
laserRetro variable copy
1d099b5 
Signed-off-by: Guillaume Doisy <[email protected]>
@chapulina @scpeters
Add Heightmap class (gazebosim#388)
2ae6fc7 
Signed-off-by: Louise Poubel <[email protected]>

Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Guillaume Doisy <[email protected]>
restore collision
28dd4c7 
Signed-off-by: Guillaume Doisy <[email protected]>
add integration test
f3f81a2 
Signed-off-by: Guillaume Doisy <[email protected]>
Guillaume Doisy added 2 commits January 6, 2021 21:13
@doisyg
Copy link
Contributor Author

doisyg commented Jan 6, 2021

Should be signed now. Removed all Collision modifications (the .hh changes were residual) as I want to implement laser_retro only in Visual for now. More simple as there is no equivalent to https://github.com/osrf/sdformat/blob/32d96a6b3587d518695253fb4d4c70b8404525b0/src/Visual.cc#L75-L83 in Collision yet

@chapulina
Copy link
Contributor

The diff looks weird, as if this PR were adding the code for #388, even though that's already merged into sdf9 🤔 Git does these weird things sometimes. I wonder if a rebase removing commit 2ae6fc7 would fix it 🤔

@doisyg
Copy link
Contributor Author

doisyg commented Jan 10, 2021

The diff looks weird, as if this PR were adding the code for #388, even though that's already merged into sdf9 thinking Git does these weird things sometimes. I wonder if a rebase removing commit 2ae6fc7 would fix it thinking

I merged the last sdf9 branch into this PR branch, this seems to have fix the diff for now. I ll do a rebase or reset if that's not enough

@doisyg doisyg mentioned this pull request Jan 10, 2021
Merged
chapulina
chapulina approved these changes Jan 23, 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.

Looking good. I still have the request to move the public functions above the private ones before merging. Thanks!

@chapulina chapulina merged commit b22a595 into gazebosim:sdf9 Jan 25, 2021
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

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@doisyg @codecov-io @chapulina