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

Replaced common::Time for std::chrono #41

Merged
merged 19 commits into from
Sep 22, 2020
Merged

Replaced common::Time for std::chrono #41

merged 19 commits into from
Sep 22, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Aug 25, 2020

This PR is part of this issue gazebosim/gz-common#61. The idea it's to deprecate the class common::Time in favor of std::chrono.

Related with gazebosim/gz-common#90

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 25, 2020
@ahcorde ahcorde requested a review from iche033 as a code owner August 25, 2020 18:10
@ahcorde ahcorde self-assigned this Aug 25, 2020
@iche033
Copy link
Contributor

iche033 commented Aug 25, 2020

hmm I think we should follow the tick-tock release cycle and deprecate the functions in Dome first then remove them in ignition E

@scpeters
Copy link
Member

it could be nice to add a helper function to ign-msgs that populates msgs::Time from std::chrono::system_clock::time_point. it could be another Convert(...) function in ignition/msgs/Utility.hh

@chapulina
Copy link
Contributor

chapulina commented Aug 25, 2020

it could be nice to add a helper function to ign-msgs that populates msgs::Time from std::chrono::system_clock::time_point

We already have some in ignition/math/Helpers.hh, so I'd suggest we add more there too.

@ahcorde ahcorde linked an issue Sep 4, 2020 that may be closed by this pull request
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Sep 5, 2020
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #41 into master will decrease coverage by 1.34%.
The diff coverage is 54.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   77.67%   76.32%   -1.35%     
==========================================
  Files          23       23              
  Lines        2302     2319      +17     
==========================================
- Hits         1788     1770      -18     
- Misses        514      549      +35     
Impacted Files Coverage Δ
include/ignition/sensors/Manager.hh 90.00% <ø> (ø)
src/Sensor.cc 88.29% <25.00%> (-4.97%) ⬇️
src/Manager.cc 75.00% <33.33%> (-6.09%) ⬇️
src/Lidar.cc 72.82% <42.85%> (-1.90%) ⬇️
src/AirPressureSensor.cc 83.33% <50.00%> (-2.59%) ⬇️
src/AltimeterSensor.cc 86.04% <50.00%> (-2.19%) ⬇️
src/CameraSensor.cc 73.35% <50.00%> (-1.56%) ⬇️
src/LogicalCameraSensor.cc 87.14% <50.00%> (-2.72%) ⬇️
src/MagnetometerSensor.cc 86.20% <50.00%> (-2.17%) ⬇️
src/ImuSensor.cc 88.46% <66.66%> (-1.74%) ⬇️
... and 4 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 f57b75e...5968527. Read the comment docs.

@ahcorde
Copy link
Contributor Author

ahcorde commented Sep 17, 2020

I can see some Ogre errors:

22: unknown file: Failure
22: C++ exception with description "OGRE EXCEPTION(2:InvalidParametersException): All framebuffer formats with this texture internal format unsupported in GL3PlusFrameBufferObject::initialise at /var/lib/jenkins/workspace/ogre-2.1-debbuilder/repo/RenderSystems/GL3Plus/src/OgreGL3PlusFrameBufferObject.cpp (line 229)" thrown in the test body.

@iche033 any thoughts?

@iche033
Copy link
Contributor

iche033 commented Sep 17, 2020

I started seeing those errors happening recently on our r2d2 jenkins machine. For other PRs, I just take it offline and rerun the build so the tests are run on a different machine.

cc current build farmer @caguero

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.

The public functions need to be deprecated. Be sure to add a note to the migration guide too. We don't usually test the deprecated functions so I think the tests can remain as they are.

include/ignition/sensors/AirPressureSensor.hh Show resolved Hide resolved
@chapulina chapulina removed the needs upstream release Blocked by a release of an upstream library label Sep 18, 2020
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.

Still missing some deprecations, but I think we're very close

include/ignition/sensors/CameraSensor.hh Show resolved Hide resolved
include/ignition/sensors/DepthCameraSensor.hh Show resolved Hide resolved
include/ignition/sensors/GpuLidarSensor.hh Show resolved Hide resolved
include/ignition/sensors/Lidar.hh Show resolved Hide resolved
include/ignition/sensors/Manager.hh Show resolved Hide resolved
include/ignition/sensors/RgbdCameraSensor.hh Show resolved Hide resolved
include/ignition/sensors/Sensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/Sensor.hh Outdated Show resolved Hide resolved
src/Sensor_TEST.cc Outdated Show resolved Hide resolved
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 just have the 2 minor comments below that I'll commit now. Then wait for CI and merge.

CC @iche033

include/ignition/sensors/Manager.hh Outdated Show resolved Hide resolved
src/Sensor.cc Outdated Show resolved Hide resolved
@chapulina chapulina merged commit f0707c9 into master Sep 22, 2020
@chapulina chapulina deleted the ahcorde/std_chrono branch September 22, 2020 14:56
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 🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update API to use std::chrono
4 participants