Improve `sf::Time` interoperability with `<chrono>`

[ChrisThrasher]

Description

This PR implements the ideas outlined here. 100% code coverage is maintained for sf::Time.

One important design note is how the constructor from std::chrono::duration does not perform a duration_cast. This is intentional because I don't want to make it too easy for users to provide a duration type that requires a lossy conversion. The onus is on the user to do that lossy conversion.

[codecov[bot]]

Codecov Report

Merging #2133 (9b4efc7) into master (72d8803) will increase coverage by 0.04%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   13.90%   13.94%   +0.04%     
==========================================
  Files         189      189              
  Lines       15813    15821       +8     
  Branches     4169     4179      +10     
==========================================
+ Hits         2199     2207       +8     
  Misses      13460    13460              
  Partials      154      154              

Impacted Files	Coverage Δ
include/SFML/System/Time.inl	100.00% <100.00%> (ø)
src/SFML/System/Clock.cpp	100.00% <100.00%> (ø)

---

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 72d8803...9b4efc7. Read the comment docs.

[ChrisThrasher]

@kimci86

The private constructor Time(Int64 microseconds) could be removed as it is used only in microseconds function which could use the new constructor instead.

Good point. I'll remove that.

Also, the class documentation could mention that conversion to/from duration is supported.

Right I forgot that. I'll add some extra information to that section describing the new functionality.

@Bromeon

I'm not sure if we should change the member variable though. What do we effectively gain? Code becomes quite a bit more verbose (mainly because duration_cast is such an ugly API).

It makes it easier to remove magic numbers from the implementation and gives us the added assurance that <chrono>'s type safety brings.

Does sizeof(Time) also increase?

No I don't believe so. std::chrono::microseconds will use a signed integer with at least 55 bits which means any sensible implementation is using a std::int64_t which is definitionally the same as sf::Int64.

[ChrisThrasher]

I squashed some commits too since I thought 4 or 5 commits was excessive for a change of this scale.

[ChrisThrasher]

What do we think about a user-defined conversion function? This would do the exact opposite of the new constructor template allowing for implicit (yet typesafe) conversions back to durations.

template <typename Rep, typename Period>
constexpr operator std::chrono::duration<Rep, Period>() const { return m_microseconds; }

EDIT: I have this implemented and tested locally. It seems to work exactly as we'd expect. I can push this up if the team wants to see it (and let CI verify it) before making a decision.

[Bromeon]

Implicit conversions between sf::Time and std::chrono::duration are probably OK, since both are already "duration" types and there's less that can go wrong than when implicitly converting to seconds/milliseconds. But I would say we make either both conversion constructor and operator implicit, or none, for symmetry.

[ChrisThrasher]

I went ahead and added the implicit conversion to std::chrono::duration.

[ChrisThrasher]

No changes. Just keeping up with master.

[ChrisThrasher]

Rebased on master and fixed new compilation error introduced by a57640c2c89042d7ecc2c5f87712b553c8e14612

[Bromeon]

This is still unaddressed:

Another thing that came to my mind: how good is debugger support now across compilers? Before, it was possible to see something like sf::Time { m_microseconds: 323811 } in debug watches and always know what's going on. Is the new internal chrono::duration field displayed as a deeply nested, barely decipherable type, or is there decent support for debug printing?

Because as mentioned in the early review, I don't see a practical advantage of storing std::duration internally -- yes, we remove magic numbers (which are quite obvious in that context), but introduce very hard-to-read chrono shenanigans. So let's at least make sure that usability of sf::Time doesn't deteriorate.

[ChrisThrasher]

but introduce very hard-to-read chrono shenanigans

To be fair, this is idiomatic <chrono> code. However verbose the API is, anyone familiar with C++11 time utilities will know what is going on.

So let's at least make sure that usability of sf::Time doesn't deteriorate.

In the VS Code debugger I can still easily see the value of the duration.

[ChrisThrasher]

I changed how we're printing standard types such that we never have to open up namespace std since I'm not 100% sure that's legal.

[kimci86]

Now it only needs a small comment to explain why we use StringMaker for stringification.

[ChrisThrasher]

Now it only needs a small comment to explain why we use StringMaker for stringification.

Done

[ChrisThrasher]

Rebased and reformatted because of #2159

[ChrisThrasher]

With Bromeon's and Vittorio's approval, is there anything else we're waiting on before this is good to merge?

[ChrisThrasher]

Rebased on master, fixed a compiler warning caused by Doctest changes, and found somewhere to use this new implicit chrono::duration->sf::Time conversion to simplify code.

[ChrisThrasher]

Added a missing & on a parameter and rebased on master.

[ChrisThrasher]

@Bromeon would you mind checking this out?

[ChrisThrasher]

Rebased on master and added an assert to handle a potential edge case that could cause UB in the tests.

[eXpl0it3r]

Woho! Thanks for this addition 🥳

[PierceLBrooks]

This change may be related to a a master branch compilation issue I am now seeing on Windows:

C:\UserData\SFML\include\SFML/System/Time.inl(223,42): error C2039: 'int64_t': is not a
member of 'std' [C:\UserData\SFML\build\src\SFML\System\sfml-system.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.28.29333\include\chrono(24): message : s
ee declaration of 'std' [C:\UserData\SFML\build\src\SFML\System\sfml-system.vcxproj]

[ChrisThrasher]

Very possible there's simply a missing #include <cstdint> somewhere. Can you share how you can reproduce this error?

[PierceLBrooks]

@ChrisThrasher Of course! My steps were as follows:

git clone https://github.com/SFML/SFML.git
cd SFML
git checkout master
mkdir build
cd build
cmake ..
cmake --build .

However, this assumes that Visual Studio 2019 will be the default pick of your system, so you may need to specify the appropriate -G flag for CMake.

[ChrisThrasher]

That's it? Weird, because we already test VS 2019 in CI. Anyways, should still be an issue fix. Mind filing an issue about this so we don't lose track of this bug? Thanks!

[PierceLBrooks]

Will do, I can try to address it with a pull request later on as well if I can track down where the cstdint include belongs! FYI, checking out the 2.5.1 release version branch allows the build to succeed just fine.

[PierceLBrooks]

https://github.com/SFML/SFML/issues/2211#issue-1381582926