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

Allow RenderBuffer::Resolve() to change the size of the image #1236

Merged

Conversation

ablev-dwa
Copy link
Contributor

Description of Change(s)

Moves aov buffer resolve ahead of outputBuffer allocation to ensure correct buffer sizing in the event of image size change

Fixes Issue(s)

This was actually working before, but it looks like it is possible for
this to allocate outputBuffer at the wrong size and write over the end
of it.

This was actually working before, but it looks like it is possible for
this to allocate outputBuffer at the wrong size and write over the end
of it.
@c64kernal
Copy link
Contributor

Thanks for this, @ablev-dwa -- it looks good! We were wondering though if you had a specific scenario when this happened? (Or even better yet a test case!) We're trying to figure out if we need to fix it elsewhere too. Thanks!

@ablev-dwa
Copy link
Contributor Author

@spitzak found this during his Hydra delegate work, I'll ask him to comment here.

@spitzak
Copy link

spitzak commented Jun 12, 2020

Two reasons:

  1. Our renderer can "downrez" and draw at a smaller size, and if I return these smaller render buffers usdview draws it correctly (by scaling it up to fit the window). However I would like to defer the calculation of the image size until the renderer runs, to avoid any errors with choices about how to round, etc.

  2. When resizing the usdview window, it would be really nice to resize the previous image until the renderer has made a "good enough" image. I'm doing this by leaving the render buffer at the old size until the new image is ready. Before 20.5 usdview did an ok job resizing this to the viewport, but it does seem to be broken in 20.5 where it is confusing the current size with the expected size (for some reason this does not break the downrez, can't figure that one out). It would be nice to fix this and it could be improved by assuming the old image is drawn with the same camera and fit rules and resizing that instead of distorting the image.

@spitzak
Copy link

spitzak commented Jun 12, 2020

A good test would be to simulate a "slow" renderer. Modify an existing renderDelegate so after the first render completes, it stops changing the size of the RenderBuffer or the image in it. Then resize the usdview viewport. Ideally the user sees the previous image scaled without distortion to the same position the real render will be.
This is working for downrez at least (resizing the window is messing up in 20.05, and distorted the image previously). But this requires Resolve to change the buffer size and it is pretty obvious in the code that if this happens the _outputBuffer will be allocated at the wrong size, so this patches that.

@jtran56
Copy link

jtran56 commented Jun 12, 2020

Filed as internal issue #USD-6122.

@c64kernal
Copy link
Contributor

Thanks @spitzak! Super helpful! This change looks good and we'll merge it, but we think it'll be pretty fragile to rely on this. A better solution might be to have the renderer produce whatever size image but always scale it to the correct HdRenderBuffer size during Resolve – and not change that size. We recognize that the current API makes that a bit confusing though and we may want to tighten that up in the future (e.g., make it super clear where the size is allowed to change).

@spitzak
Copy link

spitzak commented Jun 15, 2020

I believe it is required that usdview resize images to fit the viewport, otherwise dynamic resizing of the viewport window is impossible without blocking until the renderer returns an image.

@c64kernal
Copy link
Contributor

A big issue is that our API doesn't make this at all clear, I hope we'll address this, but the fundamental problem, we think, is that there is a mismatch between what's produced in the HdRenderBuffer and what was asked for. In the case of a progressive renderer, some of our internal tools make a distinction between the render resolution (which stays fixed, at least during a full frame cycle) and the resolution of the presentation buffer -- which dynamically changes with viewport size, for example.

At any rate, thank you again for the great feedback on this. We've merged this change, thanks also for that! And like I said, I hope in the future we can make our APIs clearer so that we can make it easier to use it in a robust way by default.

@pixar-oss pixar-oss merged commit 3ac64b8 into PixarAnimationStudios:dev Jun 30, 2020
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.

6 participants