ValveSoftware / openvr

OpenVR SDK
http://steamvr.com
BSD 3-Clause "New" or "Revised" License
6.13k stars 1.28k forks source link

Improve API safety and standard-conformance by using Modern C++ #1315

Open vittorioromeo opened 4 years ago

vittorioromeo commented 4 years ago

OpenVR uses Modern C++ constructs already (see #1314), but the API is suboptimal in terms of safety. Some examples:

// Current:
static const uint32_t k_nSteamVRVersionMajor = 1;

// Proposed:
static constexpr uint32_t k_nSteamVRVersionMajor = 1;
// Current:
typedef uint32_t SpatialAnchorHandle_t;

// Proposed:
using SpatialAnchorHandle_t = std::uint32_t;
// Current:
enum EVREye
{
    Eye_Left = 0,
    Eye_Right = 1
};

// Proposed
enum class EVREye : int
{
    Left = 0,
    Right = 1
}; 
// Current:
#define INVALID_SHARED_TEXTURE_HANDLE   ((vr::SharedTextureHandle_t)0)

// Proposed:
static constexpr vr::SharedTextureHandle_t invalidSharedTextureHandle{nullptr};
// Current:
#define VR_COMPOSITOR_ADDITIONAL_PREDICTED_FRAMES( timing ) ( ( ( timing ).m_nReprojectionFlags & vr::VRCompositor_PredictionMask ) >> 4 )

// Proposed:
[[nodiscard]] constexpr std::uint32_t getCompositorAdditionalPredictedFrames(const Compositor_FrameTiming& timing)
{
    return (timing.m_nReprojectionFlags & vr::VRCompositor_PredictionMask) >> 4;
}
// Current:
/** Gets the result of the distortion function for the specified eye and input UVs. UVs go from 0,0 in 
    * the upper left of that eye's viewport and 1,1 in the lower right of that eye's viewport.
    * Returns true for success. Otherwise, returns false, and distortion coordinates are not suitable. */
    virtual bool ComputeDistortion( EVREye eEye, float fU, float fV, DistortionCoordinates_t *pDistortionCoordinates ) = 0;

// Proposed:
/** Gets the result of the distortion function for the specified eye and input UVs. UVs go from 0,0 in 
    * the upper left of that eye's viewport and 1,1 in the lower right of that eye's viewport.
    * Returns true for success. Otherwise, returns false, and distortion coordinates are not suitable. */
    [[nodiscard]] virtual bool ComputeDistortion( EVREye eEye, float fU, float fV, DistortionCoordinates_t *pDistortionCoordinates ) = 0;

All the above are changes that do not require any standard library support, and can really improve the readability and safety of the API (especially [[nodiscard]]). There are also other possible improvements that require library support, like usage of std::string_view instead of const char* or std::string. That could lead to performance improvements in cases like the following:

// Current:
void SetString( const std::string & sSection, const std::string &  sSettingsKey, const std::string & sValue, EVRSettingsError *peError = nullptr )
        {
            m_pSettings->SetString( sSection.c_str(), sSettingsKey.c_str(), sValue.c_str(), peError );
        }

// Proposed:
void SetString( std::string_view sSection, std::string_view sSettingsKey, std::string_view sValue, EVRSettingsError *peError = nullptr )
        {
            m_pSettings->SetString( sSection.c_str(), sSettingsKey.c_str(), sValue.c_str(), peError );
        }

I could make a fork of the API targeted to Modern C++ users, but I'd rather improve openvr instead of branching out. I am happy to contribute, but first I'd like to know what your policy is and see the outcome of #1314.

vittorioromeo commented 4 years ago

Note that most of the changes above are C++11 friendly. [[nodiscard]] unfortunately isn't, but pretty much every compiler has an alternative which works even in C++03 mode. E.g. https://stackoverflow.com/questions/4226308/msvc-equivalent-of-attribute-warn-unused-result