Conditional macro required to build with vsg 1.0 and vsg 1.1

[rhabacker]

Describe the bug
In vsg 1.1, there was an api change to vsg::FrameStamp::create() that added a third parameter. This forces clients to add a condition to be able to build with vsg 1.0 as well.

To Reproduce
Steps to reproduce the behavior:

1. look at the problem reported at build error with actual vsg no matching function for call to 'vsg::FrameStamp::FrameStamp(...) geefr/vsgvr#53.
2. investigate rhabacker/vsgvr@aead073 to see the changes required to build both versions mentioned.

Expected behavior
A default parameter for the third argument would avoid the need for such differences.

[robertosfield]

It's not possible to use a default parameter and have the simulationTime set to something sensible. Even just having a second constructor that matches the old API would be left with no way to set the simulationTime to some valid.

For most application I would expect them to just let the Viewer::advatanceToNextFrame() set the FrameStamp and as the implementation is contained with the Viewer.cpp this won't affect client applications. The only time I can see the constructor change being a issue is when users are creating and using their own FrameStamp.

What is the actual code in vsgvr that is causing problems?

[robertosfield]

I should add the new FrameStamp::simulationTime is part of the Animation work merged with the 1.1.x branch. simulationTime is something missing from the VSG compared to the OSG, but now it's caught up in this regard.

[rhabacker]

What is the actual code in vsgvr that is causing problems?

See https://github.com/geefr/vsgvr/blob/aead073464dfc3143ac35f2709cfdecca0a50f40/vsgvr/src/vsgvr/app/Viewer.cpp#L110

[robertosfield]

I'm not familiar with vsgvr but the code looks kinda hacky, can't help but feel that isn't probably the cleanest way to implement what is required.

The compile workaround is for the addition of FrameStamp.simulationTime is broken, it should be passing the simulationTimem without this non of the VSG's new animation functionality will work.

Personally I'd recommend letting Viewer::advatanceToNextFrame(..) do it's thing and set everything that is required and work in a more cooperative way with the VSG rather than digging into controlling things that are probably left to the VSG to manage, If there is work that needs to be done for the VR side then one can subclass from Viewer and override methods but call the underlying Viewer method to do the work it needs to do. Taking the approach that vsgvr has taken really opens up to it breaking when the underlying functionality evolves.

[robertosfield]

I'm going to close this Issue as the VSG is doing the right thing, not allowing code to compile that would break the VSG in difficult to determine ways. This issue is with vsgvr doing things wrong.

[rhabacker]

I'm not familiar with vsgvr but the code looks kinda hacky, can't help but feel that isn't probably the cleanest way to implement what is required.

The compile workaround is for the addition of FrameStamp.simulationTime is broken, it should be passing the simulationTimem without this non of the VSG's new animation functionality will work.

Thanks for your quick review. I personally tried to simply fix a build error I had with vsgvr and vsg 1.1 and came to this topic.

Personally I'd recommend letting Viewer::advatanceToNextFrame(..) do it's thing and set everything that is required and work in a more cooperative way with the VSG rather than digging into controlling things that are probably left to the VSG to manage, If there is work that needs to be done for the VR side then one can subclass from Viewer and override methods but call the underlying Viewer method to do the work it needs to do. Taking the approach that vsgvr has taken really opens up to it breaking when the underlying functionality evolves.

The viewer class in vsgvr was derived from vsg::Viewer (see https://github.com/geefr/vsgvr/blob/aead073464dfc3143ac35f2709cfdecca0a50f40/vsgvr/include/vsgvr/app/Viewer.h#L58).
When I look at the differences in the source codes, it becomes apparent that a different time source [1] is used in vsgvr than in vsg [2], since the headset managed with openXR specifies the timing and synchronization.

[1] https://github.com/geefr/vsgvr/blob/aead073464dfc3143ac35f2709cfdecca0a50f40/vsgvr/src/vsgvr/app/Viewer.cpp#L113
[2] https://github.com/vsg-dev/VulkanSceneGraph/blob/78fd6f67f07f68e807077d20748295a74a8dc025/src/vsg/app/Viewer.cpp#L171C2-L171C35

The relevant source code is

    _frameState = XrFrameState();
    _frameState.type = XR_TYPE_FRAME_STATE;
    _frameState.next = nullptr;

    // TODO: Return statuses - session/instance loss fairly likely here
    xr_check(xrWaitFrame(_session->getSession(), nullptr, &_frameState));
    xr_check(xrBeginFrame(_session->getSession(), nullptr));

    vsg::clock::time_point t(std::chrono::nanoseconds(_frameState.predictedDisplayTime));

As far as I can tell, the base method could be used with respect to the time source if the retrieval of the time source could be overridden.

Another difference at https://github.com/geefr/vsgvr/blob/aead073464dfc3143ac35f2709cfdecca0a50f40/vsgvr/src/vsgvr/app/Viewer.cpp#L147 is that additional layers are managed here:

    for (auto& layer : compositionLayers)
    {
      layer->advanceToNextFrame();
    }

where in vsg at this location there is

   for (auto& task : recordAndSubmitTasks)
    {
        task->advance();
    }

Possibly the latter code fragment could be moved to a virtual method that could be replaced in vsgvr.

[robertosfield]

I'm afraid I'm not in a position to go review vsgxr and figure out things. The best I can do is recommend you have a look at the merge of the Animation branch for what changes were made to Viewer to enable animation.

#1094

[rhabacker]

The viewer class in vsgvr was derived from vsg::Viewer (see https://github.com/geefr/vsgvr/blob/aead073464dfc3143ac35f2709cfdecca0a50f40/vsgvr/include/vsgvr/app/Viewer.h#L58).

Further investigating showed me, that I need to correct that statement as I did found this:

 * This class is similar to vsg::Viewer, however:
 * * The event handling loop is somewhat different, requiring the application to behave correctly, based on the result of Viewer::pollEvents
 * * This viewer has no presentation surface or vsg::Window association - Rendered data is passed to OpenXR only
 *   * Any elements in vsg which require a vsg::Window cannot work with this viewer
 *   * While some functions are similar, this class does not directly inherit from vsg::Viewer
 */

[rhabacker]

#1094
Thanks for this pointer - I updated the patch with the associated changes, see geefr/vsgvr@340faa0