OGSMOD-7803 - MayaHydra - Add framing to the public API #41
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds framing functionality to the public API along with supporting unit tests to validate the implementation.
- Adds new baseline test image for frame display testing on macOS
- Implements framing capabilities in the public API
- Includes unit tests to demonstrate and verify framing functionality
hodoulp
changed the title
debloip-adsk
left a comment
debloip-adsk
left a comment
There was a problem hiding this comment.
I tested it and it does work for our purposes. However, I am wondering if there is a benefit to having the framing param be part of the ViewParams/viewInfo, when it is simply used to set renderParams.framing, which is already accessible?
My point is that it makes the API more understandable / readable i.e., consumer can set exactly what the context needs (nothing to guess). |
Signed-off-by: Patrick Hodoul <[email protected]>
|
cc'ing @pixnblox, @erikaharrison-adsk and/or @sebastienberube-adsk can you have a look? |
kohakukun
left a comment
kohakukun
left a comment
There was a problem hiding this comment.
LGTM in general but, as this is open source, we might want to keep a changelog since this is a breaking change. This change would increase the major version
| TestHelpers::FramePassInstance::CreateInstance(stage.stage(), context->_backend); | ||
|
|
||
| // Render 10 times (i.e., arbitrary number to guaranty best result). | ||
| int frameCount = 10; |
There was a problem hiding this comment.
Why do we need to render multiple times? Is there something like noise cancelling or other statistical methods happening in the rendering?
There was a problem hiding this comment.
Sometime you need several passes to have a 'complete' render when using Storm.
| ASSERT_TRUE(context->_backend->compareImages(imageFile)); | ||
| } | ||
|
|
||
| TEST(TestViewportToolbox, TestFramePasses_DisplayClipping1) |
There was a problem hiding this comment.
If I understand correctly, this is the use case which was not possible with only the viewport?
There was a problem hiding this comment.
Yep. But it was possible by directly accessing the underneath render pass. Now it's easier to access and customize.
|
|
||
| params.renderBufferSize = GfVec2i(width, height); | ||
| params.viewInfo.viewport = { { 0, 0 }, { width, height } }; | ||
| params.renderBufferSize = GfVec2i(width, height); |
There was a problem hiding this comment.
Nit: I would use the style that was used before and is being used in the other lines. With the spaces to generate alignment. This also will reduce the list of changes
There was a problem hiding this comment.
This line still aligns with the line below which changed.
include/hvt/engine/framePass.h
Outdated
| /// Gets the viewport dimensions. | ||
| virtual const PXR_NS::GfVec4i GetViewport() const | ||
| /// Gets the viewport positions & dimensions. | ||
| virtual const PXR_NS::GfRange2f GetViewport() const |
There was a problem hiding this comment.
IMO since we are moving away from the viewport keyword, we might change the getter as well and have maybe GetDisplayWindow and GetDataWindow
There was a problem hiding this comment.
Lot of impacts on existing code but it's a good idea.
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
The pull request adds the framing in the public API and unit tests to demonstrate that it works.
In fact, it replaces the former
ViewportportRectby the genericpxr::CameraUtilFramingintroducing lot of changes. I still picked that solution to have the most generic method directly in the public API.Note: There are some changes in all the unit tests (explained the number of changed files in this pull request). But these changes are quite simple and there are two helpers to make that transition even easier.