Gpu Time Correlation

[GrantComm]

Highlights corresponding VkBeginCommandBuffer, Frame, or VkBeginRenderPass node when item is selected in the gpu time tab.

gpu_time_correlation.mp4

[angela28chen]

I'm a bit confused by the PR description that says to start reviewing from 92e7b2f, it seems like there are important changes from before that point? Is this PR changes based off another pending PR?

EDIT: I checked #389 and it seems like there are two additional commits in this change, so should I be reviewing beginning from 4f6b8ef?

[GrantComm]

I'm a bit confused by the PR description that says to start reviewing from 92e7b2f, it seems like there are important changes from before that point? Is this PR changes based off another pending PR?

EDIT: I checked #389 and it seems like there are two additional commits in this change, so should I be reviewing beginning from 4f6b8ef?

Sorry yes, please start at 4f6b8ef. Updated the description. This change is based on #389

[angela28chen]

I spent some time thinking about your proposed setup with m_gpu_time_correlation_from_command_hierarchy to explicitly block signals and solve the issue of having infinite signal loop triggered by the selection changing for these related tabs (vulkan command hierarchy, pm4 tree, gpu timing). I think rather than use this approach, we should consider the fact that the currentChanged signal should only be produced when the selected element has changed. To give an example:

1. User clicks on GPU time row and GpuTimingTabView::OnSelectionChanged is triggered
2. Emits GpuTimingTabView::GpuTimingDataSelected for a valid row
3. MainWindow::OnGpuTimingDataSelected is triggered
4. Selection is cleared for other views (vulkan and pm4)
5. If a vulkan row that correlates with the gpu row is found, then the vulkan table setCurrentIndex() is called
6. Eventually that comes back around to triggering GpuTimingTabView::OnEventSelectionChanged, looping back to step 1

I think the issue here is step 4. If we are more sparing about clearing selection, then step 5 may not trigger a currentChanged if the selection is the same as before. My guess is that if we set it up so that the selection is cleared ONLY when a correlated row is not found, this would terminate the loop once the selections (valid row or none) across the 3 tabs are consistent with each other. I think this should apply to the relationship between the vulkan and pm4 tabs too.

Please let me know what you think.

https://doc.qt.io/qt-6/qitemselectionmodel.html#currentChanged

[GrantComm]

I spent some time thinking about your proposed setup with m_gpu_time_correlation_from_command_hierarchy to explicitly block signals and solve the issue of having infinite signal loop triggered by the selection changing for these related tabs (vulkan command hierarchy, pm4 tree, gpu timing). I think rather than use this approach, we should consider the fact that the currentChanged signal should only be produced when the selected element has changed. To give an example:

1. User clicks on GPU time row and GpuTimingTabView::OnSelectionChanged is triggered
2. Emits GpuTimingTabView::GpuTimingDataSelected for a valid row
3. MainWindow::OnGpuTimingDataSelected is triggered
4. Selection is cleared for other views (vulkan and pm4)
5. If a vulkan row that correlates with the gpu row is found, then the vulkan table setCurrentIndex() is called
6. Eventually that comes back around to triggering GpuTimingTabView::OnEventSelectionChanged, looping back to step 1

I think the issue here is step 4. If we are more sparing about clearing selection, then step 5 may not trigger a currentChanged if the selection is the same as before. My guess is that if we set it up so that the selection is cleared ONLY when a correlated row is not found, this would terminate the loop once the selections (valid row or none) across the 3 tabs are consistent with each other. I think this should apply to the relationship between the vulkan and pm4 tabs too.

Please let me know what you think.

https://doc.qt.io/qt-6/qitemselectionmodel.html#currentChanged

I have updated the correlation and removed m_gpu_time_correlation_from_command_hierarchy. I still think the signals should be blocked due to certain cases. For example, in the case where a gpu time node is selected after draw correlation has occurred. In that case the views would be cleared and if there is a corresponding begincmdbuffer/beginrenderpass node it would be selected, if not just the selected node would be highlighted. Without the signal blocking, the clearing of the pm4 command hierarchy selection would trigger CorrelatePm4DrawCall() which then clears the selection of the gpu time tab which is unwanted.

I changed the use of booleans to QSignalBlocker on the selection models. It blocks the signal on the selection model until it falls out of scope. https://doc.qt.io/qt-6/qsignalblocker.html

[angela28chen]

Thank you for discussing this with me, I think this PR is fine for now but we should definitely look into a refactor for the correlation stuff down the line.