Proposal for simplified version checking

[rhabacker]

Is your feature request related to a problem? Please describe.

In order to support the old and new version for API changes of vsg, e.g. when changing the include path for Light.h to include/vsg/lighting/Light.h, the following code snippet is required:

#if VSG_VERSION_MAJOR >= 1 && VSG_VERSION_MINOR >= 1 && VSG_VERSION_PATCH >= 3 
#include <vsg/lighting/Light.h>
#else
#include <vsg/nodes/Light.h>
#endif

Describe the solution you'd like

With the definition of two additional macros in include/vsg/core/Version.h

#define VSG_VERSION_CHECK(major, minor, patch) ((major<<16)|(minor<<8)|(patch))
#define VSG_VERSION VSG_VERSION_CHECK(VSG_VERSION_MAJOR, VSG_VERSION_MINOR, VSG_VERSION_PATCH)

this is simplified to

...
#include <vsg/core/Version.h> 
...
#if VSG_VERSION >= VSG_VERSION_CHECK(1, 1, 3)
#include <vsg/lighting/Light.h>
#else
#include <vsg/nodes/Light.h>
#endif

as this is similarly supported in Qt.

[robertosfield]

Could you not just include all.h?

…

On Mon, 6 May 2024, 14:05 Ralf Habacker, ***@***.***> wrote: *Is your feature request related to a problem? Please describe.* In order to support the old and new version for API changes of vsg, e.g. when changing the include path for Light.h to include/vsg/lighting/Light.h, the following code snippet is required: #if VSG_VERSION_MAJOR >= 1 && VSG_VERSION_MINOR >= 1 && VSG_VERSION_PATCH >= 3 #include <vsg/lighting/Light.h> #else #include <vsg/nodes/Light.h> #endif *Describe the solution you'd like* With the definition of two additional macros in include/vsg/core/Version.h #define VSG_VERSION_CHECK(major, minor, patch) ((major<<16)|(minor<<8)|(patch)) #define VSG_VERSION VSG_VERSION_CHECK(VSG_VERSION_MAJOR, VSG_VERSION_MINOR, VSG_VERSION_PATCH) this is simplified to #if VSG_VERSION >= VSG_VERSION_CHECK(1, 1, 3) #include <vsg/lighting/Light.h> #else #include <vsg/nodes/Light.h> #endif as this is similarly supported in Qt. — Reply to this email directly, view it on GitHub <#1184>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKEGUD4GBTDE74BKALYIH3ZA55YLAVCNFSM6AAAAABHJAES76VHI2DSMVQWIX3LMV43ASLTON2WKOZSGI4DAOBTHA4TKMY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[robertosfield]

Hi @rhabacker,,

I was away over the weekend but am back at work now so following up properly.

I've looked into how Qt and Vulkan do it, and they both take a similar approach as you've suggested.

The QT_VERSION_CHECK wording is awkward, VK_MAKE_API_VERSION is perhaps a bit better but still awkward.

The VSG also has the VsgVersion struct:

    struct VsgVersion
    {
        unsigned int major;
        unsigned int minor;
        unsigned int patch;
        unsigned int soversion;
    };

We also have a < operator for it that can be used at runtime. Could constexpr be used with this?

The vsg::Input and vsg::Ouput have a version chec methods in the form:

       VsgVersion version;

        virtual bool version_less(uint32_t major, uint32_t minor, uint32_t patch, uint32_t soversion = 0) const;
        virtual bool version_greater_equal(uint32_t major, uint32_t minor, uint32_t patch, uint32_t soversion = 0) const;

Perhaps macro equivalents of these would be appropriate. The OSG has macros that roughly take this approach.

Whatever we end up with for the VSG it'll need to be coherent with the existing functionality rather than spinning off with another orthogonal approach - the bit operators approach is orthogonal to the VsgVersion struct approach so I'm hesitant to adopt it as well. We'd either use or the other,

Separate from this, the suggestion of adding ifdef paths into the Version.h header just to handle the move of the vsg::Light class into it's own sub-directory is a recipe for long term hacks building up in this header. When the API changes we can only go so far in insulating users from changes. Each of the required remapping would be specific to the API changed so there wouldn't be one consistent way would try to support this.

In this particular instance just include all.h will work on all VSG versions to handle any changes in placement of headers. all.h has existed from the the very early days of the VSG, well before 1.0 was made.

[robertosfield]

I tried out the Qt/Vulkan approach of using bit operators but I couldn't get it to work with the same types that VsgVersion struct uses so couldn't get something that was compatible. I ended up implementating something similar to what the OSG uses, merged it with PR #1185.

With your code segment you'd write it:

#if VSG_API_VERSION_GREATER_EQUAL((1, 1, 3)
#include <vsg/lighting/Light.h>
#else
#include <vsg/nodes/Light.h>
#endif