-
Notifications
You must be signed in to change notification settings - Fork 30
Update glTFViewer for render passes and depth stencil state #39
Update glTFViewer for render passes and depth stencil state #39
Conversation
examples/glTFViewer/glTFViewer.cpp
Outdated
.SetRenderPass(renderpass) | ||
// attachment 0 -> back buffer | ||
// (implicit) // TODO([email protected]): use the texture provided by WSI | ||
.SetDimensions(640, 480) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's weird to me that screen dimensions are now part of the material info. But I think it's correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of the material info? But I think it makes sense because you need DS buffers etc. Of the same size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaterialInfo is a struct I have in the glTFViewer. If we wanted to be able to resize the viewport, the materials would all have to be recompiled, basically, but that seems right. glTF 2.0 probably solves this, anyway (just one pipeline by default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand: the pipeline depends on the subpass, but it is the framebuffer that has the dimension, and in all APIs, the viewport is a dynamic state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, not the actual "viewport", I mean (essentially) the backbuffer size. Whatever size you want to render at.
But I think I misunderstood myself and this doesn't have to be the case. I'll figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I figured out why I was being silly. This is fixed now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
examples/glTFViewer/glTFViewer.cpp
Outdated
.SetRenderPass(renderpass) | ||
// attachment 0 -> back buffer | ||
// (implicit) // TODO([email protected]): use the texture provided by WSI | ||
.SetDimensions(640, 480) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of the material info? But I think it makes sense because you need DS buffers etc. Of the same size.
examples/glTFViewer/glTFViewer.cpp
Outdated
@@ -496,6 +520,7 @@ namespace { | |||
material.uniformBuffer.SetSubData(0, | |||
sizeof(u_transform_block) / sizeof(uint32_t), | |||
reinterpret_cast<const uint32_t*>(&transforms)); | |||
cmd.BeginRenderPass(material.renderpass, material.framebuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing an EndRenderpass, can you add validation for currentRenderpass == nullptr at the end of a CommandBuffeBuilder::ValidateGetResult(); (and ideally a test for it somewhere in the validation tests.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
#7, #29