Skip to content

Remove last usage of boost::date_time and replace it with std::chrono - #5596

Merged
larshg merged 12 commits into
PointCloudLibrary:masterfrom
larshg:removeboostdep
Feb 20, 2023
Merged

Remove last usage of boost::date_time and replace it with std::chrono#5596
larshg merged 12 commits into
PointCloudLibrary:masterfrom
larshg:removeboostdep

Conversation

@larshg

@larshg larshg commented Feb 4, 2023

Copy link
Copy Markdown
Contributor

No description provided.

@larshg
larshg force-pushed the removeboostdep branch 3 times, most recently from 1050b49 to a3182c6 Compare February 4, 2023 17:49
@larshg larshg added this to the pcl-1.13.1 milestone Feb 4, 2023
@larshg larshg added changelog: removal Meta-information for changelog generation changelog: fix Meta-information for changelog generation labels Feb 4, 2023
@larshg

larshg commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author

If someone could test this with an actual device, it would be nice 😄

@larshg

larshg commented Feb 4, 2023

Copy link
Copy Markdown
Contributor Author

Format would be a bit different:
https://godbolt.org/z/jrKoqnrrT
Should we implement our own formatting of the filenames?

@mvieth

mvieth commented Feb 6, 2023

Copy link
Copy Markdown
Member

I think it would be good if we keep the current format.
I had the idea to use std::strftime or std::put_time to replace to_iso_string, but it seems like they don't support sub-second resolution, which we would need.
This stackoverflow question gives some ideas, maybe we can do something similar to this answer?

@larshg

larshg commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

I think it would be good if we keep the current format. I had the idea to use std::strftime or std::put_time to replace to_iso_string, but it seems like they don't support sub-second resolution, which we would need. This stackoverflow question gives some ideas, maybe we can do something similar to this answer?

Yeah, I thought about adding a common function for quering a timepoint friendly name, with that format. However the - is removed, so it only contains T and ..
I'll see when I get time 👍

@mvieth

mvieth commented Feb 6, 2023

Copy link
Copy Markdown
Member

"%Y%m%dT%H%M%S" could work to imitate the first part of to_iso_string

@larshg
larshg force-pushed the removeboostdep branch 3 times, most recently from cce60de to 99d6ce9 Compare February 8, 2023 08:53
@larshg
larshg requested a review from mvieth February 8, 2023 13:22
@mvieth

mvieth commented Feb 8, 2023

Copy link
Copy Markdown
Member

I found some more occurrences of date_time:

apps/src/openni_klt.cpp:#include <boost/date_time/posix_time/posix_time.hpp> // for to_iso_string, local_time
cmake/pcl_pclconfig.cmake:set(PCLCONFIG_AVAILABLE_BOOST_MODULES "system filesystem date_time iostreams")
common/include/pcl/common/boost.h:#include <boost/date_time/posix_time/posix_time.hpp>
gpu/kinfu_large_scale/tools/kinfuLS_app.cpp:#include <boost/date_time/posix_time/posix_time.hpp> // for microsec_clock::local_time
gpu/kinfu/tools/kinfu_app.cpp:#include <boost/date_time/posix_time/posix_time.hpp> // for microsec_clock::local_time
io/include/pcl/io/boost.h:#include <boost/date_time/posix_time/posix_time.hpp>
io/src/image_grabber.cpp:#include <boost/date_time/posix_time/posix_time.hpp> // for posix_time
visualization/include/pcl/visualization/boost.h:#include <boost/date_time/posix_time/posix_time.hpp>

@larshg
larshg force-pushed the removeboostdep branch 4 times, most recently from 6e57656 to 5978374 Compare February 9, 2023 19:36

@mvieth mvieth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The last remaining mentions of date_time I found:
In pcl_pclconfig.cmake
And the apt package libboost-date-time-dev in .dev/docker/env/Dockerfile, .dev/docker/release/Dockerfile, and .dev/docker/ubuntu-variety/Dockerfile (I would suggest to replace these by libboost-serialization-dev which was implicitly installed by libboost-date-time-dev, see https://packages.ubuntu.com/kinetic/libboost-date-time1.74-dev)

Comment thread tools/openni_save_image.cpp Outdated
<iframe title="PCL OpenNI Viewer example" width="480" height="390" src="https://www.youtube.com/embed/x3SaWQkPsPI?rel=0" frameborder="0" allowfullscreen></iframe>

So let's look at the code. From *visualization/tools/openni_viewer_simple.cpp*
So let's look at the code. From *tools/openni_viewer_simple.cpp*

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This seems to have been changed a lot compared to the openni_viewer_simple.cpp. but I just updated the sleep method and left the rest alone, since its grown quite a lot in LOCs.

Add parsing of string to time_point.

Return timestamp in local timezone instead of UTC.

rename from timestamp_generator to timestamp, since it can also parse now.
Faster find using a char instead of string, per clang-tidy.
Comment thread tools/openni_image.cpp Outdated
Comment thread test/io/CMakeLists.txt Outdated
Comment thread gpu/kinfu/tools/kinfu_app.cpp Outdated
… 1000 in kinfu.

Lowercase timestamp test in test/io/CMakeLists.txt
Call getTimestamp once in openni_image.
@larshg
larshg merged commit ad56383 into PointCloudLibrary:master Feb 20, 2023
@larshg
larshg deleted the removeboostdep branch February 20, 2023 11:59
@mvieth mvieth changed the title Remove last usage of date_time and replace it with std::chrono Remove last usage of boost::date_time and replace it with std::chrono Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: fix Meta-information for changelog generation changelog: removal Meta-information for changelog generation module: io module: tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants