search

vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.32k stars 212 forks source link

viewTraversalIndex remains 0 under 2 windows #371

Closed wangii closed 2 years ago

wangii commented 2 years ago

Describe the bug The scene graph setup:

The error is the GraphicsPipeLine only has 1 vk _implementation after compile traversal, but should have 2, thus causes crash. VUID-vkCmdBindPipeline-commonparent(ERROR / SPEC): msgNum: -1497306210 - Validation Error: [ VUID-vkCmdBindPipeline-commonparent ] Object 0: handle = 0x221c08d7bd0, type = VK_OBJECT_TYPE_INSTANCE; | MessageID = 0xa6c0eb9e | Object 0xb9181f0000000029 of type VkPipeline was not created, allocated or retrieved from the correct device. The Vulkan spec states: Both of commandBuffer, and pipeline must have been created, allocated, or retrieved from the same VkDevice (https://vulkan.lunarg.com/doc/view/1.2.189.2/windows/1.2-extensions/vkspec.html#VUID-vkCmdBindPipeline-commonparent)

Expected behavior The vsg::GraphicsPipeline object should've been created according to the number of devices but Context.viewID or BinDetail.viewTraversalIndex cause the problem. It can be solved by creating StateGroup for each view but would increase the management cost.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

robertosfield commented 2 years ago

Hi Linanm

The View's viewID should be different for each View, the Viewer::compile() should set this appropriate, but perhaps it's missing this usage case.

Do any of the VSG examples reproduce this problem?

wangii commented 2 years ago

hi Robert,

The scene setup is closed to vsgmultigpu example. it does crash with following errors with debugLayer = true:

VUID-vkCmdPushConstants-offset-01795(ERROR / SPEC): msgNum: 666667206 - Validation Error: [ VUID-vkCmdPushConstants-offset-01795 ] Object 0: handle = 0x17c509a8cb8, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x27bc88c6 | vkCmdPushConstants(): VK_SHADER_STAGE_VERTEX_BIT, VkPushConstantRange in VkPipelineLayout 0xd175b40000000013[] overlapping offset = 128 and size = 64, do not contain VK_SHADER_STAGE_VERTEX_BIT. The Vulkan spec states: For each byte in the range specified by offset and size and for each shader stage in stageFlags, there must be a push constant range in layout that includes that byte and that stage (https://vulkan.lunarg.com/doc/view/1.2.189.2/windows/1.2-extensions/vkspec.html#VUID-vkCmdPushConstants-offset-01795)
    Objects: 1
        [0] 0x17c509a8cb8, type: 6, name: NULL
VUID-vkCmdBindPipeline-commonparent(ERROR / SPEC): msgNum: -1497306210 - Validation Error: [ VUID-vkCmdBindPipeline-commonparent ] Object 0: handle = 0x17c4af67970, type = VK_OBJECT_TYPE_INSTANCE; | MessageID = 0xa6c0eb9e | Object 0xd897d90000000016 of type VkPipeline was not created, allocated or retrieved from the correct device. The Vulkan spec states: Both of commandBuffer, and pipeline must have been created, allocated, or retrieved from the same VkDevice (https://vulkan.lunarg.com/doc/view/1.2.189.2/windows/1.2-extensions/vkspec.html#VUID-vkCmdBindPipeline-commonparent)
    Objects: 1
        [0] 0x17c4af67970, type: 1, name: NULL
wangii commented 2 years ago

I think it's related to this commit: https://github.com/vsg-dev/VulkanSceneGraph/commit/5799ee6e2fa819c8ecdee060d09741420b8404bd#diff-f43866ba72c628c84dfffd4a40e3d9cc755df17d7756441f8f6880d8b061e96f

robertosfield commented 2 years ago

Thanks for the detective work. From the looks of it the CollectDescriptorStats::views container is assumed to be a single instance, but in the usage case in questions multiple CollectDescriptorStats are being created, so multiple views containers.

While I haven't had a chance to really think about this issue, my first inclination would be to copy the views container values between each CollectDescriptorStats traversals.

wangii commented 2 years ago

It's relative easy to solve the problem but I think it's related to the confusing _implementations[viewID] and _implementations[deviceID]. there is no clear rule. could you clarify the two a bit?

robertosfield commented 2 years ago

Hi Linan,

On Wed, 24 Nov 2021 at 01:32, Linan Wang @.***> wrote:

It's relative easy to solve the problem but I think it's related to the confusing _implementations[viewID] and _implementations[deviceID]. there is no clear rule. could you clarify the two a bit?

viewID was introduced to handle one scene graph being used within multiple views at one time, such as when you have map view overlaid on a 2D view. In this case you need to bind a different GraphcisPipeline for each view.

I may change how this is managed in the future, but that's the reason for existence right now.

wangii commented 2 years ago

Thanks for the explanation.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Robert Osfield @.> Sent: Wednesday, November 24, 2021 7:04:39 PM To: vsg-dev/VulkanSceneGraph @.> Cc: Linan Wang @.>; Author @.> Subject: Re: [vsg-dev/VulkanSceneGraph] viewTraversalIndex remains 0 under 2 windows (Issue #371)

Hi Linan,

On Wed, 24 Nov 2021 at 01:32, Linan Wang @.***> wrote:

It's relative easy to solve the problem but I think it's related to the confusing _implementations[viewID] and _implementations[deviceID]. there is no clear rule. could you clarify the two a bit?

viewID was introduced to handle one scene graph being used within multiple views at one time, such as when you have map view overlaid on a 2D view. In this case you need to bind a different GraphcisPipeline for each view.

I may change how this is managed in the future, but that's the reason for existence right now.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/vsg-dev/VulkanSceneGraph/issues/371#issuecomment-977768703, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAAGSCIL6QGVSAAXA2PW4DTUNTBEPANCNFSM5ITARUXA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

robertosfield commented 2 years ago

One of my tasks for today will be coming up with a fix for this issue.

robertosfield commented 2 years ago

@wangii I am looking at the issue now, I've reviewed the Viewers:compile() and it already does what I though was missing. How recent is the version of the VSG that you are using?

robertosfield commented 2 years ago

I have create a new vsgmultiwindows example that shows the bug with incorrect increments of viewID. I have got it working with some hacks in the vsg::CollectResourceRequirements code but it's not in a form suitable for merging. The new example also is ready to merge so I'll return to this topic tomorrow.

wangii commented 2 years ago

my work based on vsg commit 8028fcfcae4db5d284f987cd72930156de568764. it's a bit dated but other than few small thing it works pretty well.

robertosfield commented 2 years ago

I have refactored the way View::viewID index is managed, this is now checked into #374

Could you test VSG master to see if it addresses the issues you've seen?

wangii commented 2 years ago

sure. I'll get back to you when I finish merging vsg changes in last 30 days.

wangii commented 2 years ago

the preliminary test gets the error on IndexBuffer:

UNASSIGNED-GeneralParameterError-RequiredParameter(ERROR / SPEC): msgNum: -1711571459 - Validation Error: [ UNASSIGNED-GeneralParameterError-RequiredParameter ] Object 0: handle = 0x2cd3991ee78, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0x99fb7dfd | vkCmdBindIndexBuffer: required parameter buffer specified as VK_NULL_HANDLE
    Objects: 1
        [0] 0x2cd3991ee78, type: 3, name: NULL
VUID-vkCmdBindIndexBuffer-buffer-parameter(ERROR / SPEC): msgNum: -827237651 - Validation Error: [ VUID-vkCmdBindIndexBuffer-buffer-parameter ] Object 0: handle = 0x2cd34d63880, type = VK_OBJECT_TYPE_INSTANCE; | MessageID = 0xceb15aed | Invalid VkBuffer Object 0x0. The Vulkan spec states: buffer must be a valid VkBuffer handle (https://vulkan.lunarg.com/doc/view/1.2.189.2/windows/1.2-extensions/vkspec.html#VUID-vkCmdBindIndexBuffer-buffer-parameter)

I'll have a deeper look at it later.

wangii commented 2 years ago

The problem is due to BindIndexBuffer.cpp line 89.

I'm trying to use DeviceGroup in Device.cpp, to get away with VulkanData / implementations all together.

robertosfield commented 2 years ago

Sorry for the slow reply, we've had a big storm and power cuts over the weekend and Monday so am now just catching up.

I've created a vsgwindows example that create two windows, and enable the sharing of Instance/Device between the Windows as default by using the WindowTraits::shareWindow member, or using separate Instance/Device. The later requires VSG_MAX_DEVICES to be set to 2. This work vsgmultiwindows branch of vsgExamples and the MultiWindowTrackball branch of VulkanSceneGraph,

When I run:

 vsgwindows models/lz.vsgt

It runs with two separate windows each with their own trackball controlling them. The changes to the vsg::Trackball in the MultiWindowTrackball enable the Trackball to just respond to windows that are associated with it.

When I run:

 vsgwindows models/lz.vsgt --no-shared-window

I get a crash at VertexIndexDraw.cpp line 143.

While it's not the same error, but I suspect it's a similar issue with the BufferInfo rewrite not handling multiple vsg::Device usage. This is a separate issue to View::viewID bug that should now be addressed

robertosfield commented 2 years ago

I have now merged the MultiWindowTrackball and vsgmultiwindows branches with the VSG and vsgExamples master. I'll create a MultiDevice branch for the bug fixes that I need to come up with the resolve the BindIndexBuffer and VertexIndexDraw crashes.

robertosfield commented 2 years ago

I don't have a fix yet, but it looks to me like the way the new BufferInfo::ref_ptr parent member is be utilized is leading to the memory for the first vsg:Device being deallocated when the memory for the new vsg::Device is allocated and assigned to the BufferInfo.

I am optimistic I'll have a solution to this problem checked in this afternoon.

robertosfield commented 2 years ago

I had too many tasks away from the computer to get on with resolving the multi device bug. Will be back on the case tomorrow.

wangii commented 2 years ago

:) thanks for the update.

robertosfield commented 2 years ago

When reviewing the problem areas a good clean/satisfactory solution has not presented itself so I haven't yet implemented a solution - the problem is as much a design weakness as a implementation problem.

This morning it's occurred to me that it's time to take a step back and try a different tack to see if that might offer a cleaner solution. The approach I'm going to try today is to refactor vsg::Context and the vsg::CompileTraversal/vsg::Viewer::compile()/vsg::Command::compile(..) so that rather than having a single Device handled per vsg::CompileTraversal would be to compile the resources for all the Devices together.

This approach is a radical change as I'll need more time to design, implement and test. It may have it's own issues, but it's worth trying out a change in course to see if a better approach might improve things.

robertosfield commented 2 years ago

I've spent a number of days experimenting with a refactor of the way vsg::Context is used in the VSG, changing it to handle multiple Device at once, but the changes have ended up so extensive that I'm not comfortable with just pushing on in this direction. I feel there is value in a number of changes but the change in approach hasn't quickly coalesced around a new approach that is clear that it's the best way forward.

I think it's best for me to take a step back and tackle other issues then return to this topic with a fresh look, rather than keep digging in the trenches. The current experiment is checked into the MultiDeviceFixes branch: MultiViewDevices

wangii commented 2 years ago

Got it. I'll have 2 RTX 5000 again later this months and give it a try.

robertosfield commented 2 years ago

On Thu, 9 Dec 2021 at 03:45, Linan Wang @.***> wrote:

Got it. I'll have 2 RTX 5000 again later this months and give it a try.

Beast of a system! I just have my 2060 installed on my dev system right now. My plan is to drop in two 1650 cards and do lots of testing/debugging on the multi-gpu side as there have been regressions since I last had them in my system. This isn't my only task to do before 1.0 so I'll have to juggle this with other tasks.

robertosfield commented 2 years ago

This is my work in progress on addressing the problems with multi device compile bugs:

AlternateMultiDeviceFixes

I've created scene graphs using vsg::Commands/BindIndexBuffer/BindVertexBuffers as well a vsg::Geometry/vsg::VertexIndexDraw variants and now all work safely with vsgwindows when creating a vsg::Device per window - which should mean the multi GPU case works fine as well.

Once I've finished some further work on this branch I'll pop in my 3 Gefoce 1650 cards and test multi-gpu. If you can test the AlternateMultiDeviceFixes branch with your application and data this would also be useful test of the new code.

robertosfield commented 2 years ago

HI @wangii

I have now completed my refactoring of multi-device/gpu support and have tested it out on my system using 3 cards. The work is now merged with master PR #378 .

I have also written up this work on vsg-uers post

I will now close this Issue as it "should" be resolved. If there are issues that remain they will likely be in the refactored code and unlikely to present themselves in the same manner as reported in this thread, so opening a new Issue for any further problems would be most appropriate.

wangii commented 2 years ago

Got it, thanks

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Robert Osfield @.> Sent: Thursday, December 16, 2021 1:47:10 AM To: vsg-dev/VulkanSceneGraph @.> Cc: Linan Wang @.>; Mention @.> Subject: Re: [vsg-dev/VulkanSceneGraph] viewTraversalIndex remains 0 under 2 windows (Issue #371)

Closed #371https://github.com/vsg-dev/VulkanSceneGraph/issues/371.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/vsg-dev/VulkanSceneGraph/issues/371#event-5773051834, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAAGSCK2USSJJBZTJVMNEXTURDIB5ANCNFSM5ITARUXA. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.