Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nvidia atw updates #355

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Nvidia atw updates #355

wants to merge 6 commits into from

Conversation

JeroMiya
Copy link
Contributor

  • default thread sleep for the ATW Double buffered sample set to 0 instead of 500.
  • added log lines to indicate thread ids of the render thread and the ATW thread to help debugging.
  • D3DCAPI example now uses double buffering and single render target for both eyes.
  • added client context timeout code for D3DCAPI example.
  • Added m_vsyncBlockAfterFrameBufferPresent field to D3D base class to control vsync blocking behavior. Used by ATW to block after the framebuffer present of the wrapped RenderManager instance but not before (which is handled by the ATW thread).
  • modified constructor params of the wrapped D3D11 RenderManager instance before constructing it and passing it to the D3D11 ATW backend.
  • Added a std::condition_variable to the D3D11 ATW backend and use it to block client Present until that frame is presented.

Copy link
Contributor

@russell-taylor russell-taylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are larger blocks of change here than I can do an easy review on. I commented on one change that seems to have made the code less general.

HRESULT hr;
for (size_t i = 0; i < numRenderInfo; i++) {
for (size_t i = 0; i < 2; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this less general? A monocular view onscreen is possible when we use numRenderInfo and not when we use 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 here has a different meaning than the numRenderInfo it replaces. It's no longer the number of render info but the number of buffers in the client-side swap chain, which the client controls and which we only support 1 or 2 at the moment.

For reference, these are the only configurations we support at the moment (i.e. for RegisterRenderBuffers):
1 buffer, false for clientWillNotOverwrite: both eyes in one buffer, single buffered (texture copy)
2 buffers, false for clientWillNotOverwrite: one buffer per eye, single buffered (texture copy)
2 buffers, true for clientWillNotOverwrite: both eyes in one buffer, double buffered (NO texture copy)
4 buffers, true for clientWillNotOverwrite: one buffer per eye, double buffered (NO texture copy)

RenderManager currently has no generic way to support all scenarios and so the client has to make assumptions in order to fit into one of the four supported configurations above - e.g. that there are going to be two render infos, each corresponding to an eye. Ideally, the client would request the suggested viewports and buffer sizes and number for a given client preference (one buffer per eye or all eyes in one buffer X double buffered or not double buffered) and then RenderManager would give the client 1) how many buffers to create, 2) what sizes for each buffer, 3) which RenderInfos to render for which buffers and 4) what viewports on THOSE buffers to use (as opposed to the viewports RenderInfo has, which assumes one buffer per eye).

Copy link
Contributor

@russell-taylor russell-taylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the new code on a DK2 with a GeForce 1080 and it seemed to work the same as the master code, both on the ATW test program that idles between presents and on the HighPoly code that pushes hard on the graphics card with resulting low rendering rate.

If we can separate the changes to the CAPI demo app out, I'm glad to approve the rest of the changes. See my comments on the changes in that app.

static_cast<float>(renderInfo.viewport.width),
static_cast<float>(renderInfo.viewport.height));
context->RSSetViewports(1, &viewport);
CD3D11_VIEWPORT viewport(static_cast<float>(eye == 0 ? 0 : renderInfo.viewport.width),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be handled by RenderManager, not by the application. The viewport should have been set correctly by the osvrRenderManagerGetRenderInfoFromCollectionD3D11() call.


// Make a grey background
FLOAT colorRgba[4] = { 0.3f, 0.3f, 0.3f, 1.0f };
context->ClearRenderTargetView(renderTargetView, colorRgba);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are two windows for the two eyes, we're going to have to clear both of them here, not just one of them.

RenderView(i, renderInfo[i], renderTargetView, depthStencilView);
}

// Every other second, we show a black screen to test how
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code to do the black swap was removed, but this comment was not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants