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

Display AOVs rework #162

Merged
merged 26 commits into from
May 19, 2021
Merged

Display AOVs rework #162

merged 26 commits into from
May 19, 2021

Conversation

dedoardo
Copy link
Contributor

@dedoardo dedoardo commented May 4, 2021

This patch introduces the following changes when rendering in non-tiled mode. It requires this Cycles PR.

  • Aov's render buffers are now updated once a rendering pass has finished through the new callback
  • Blitting render buffers now does a single copy rather than 2 and is not tied to RenderPass::_Execute anymore.
  • Misc improvements to HdRenderBuffer.
    • Memory is actually deallocated on deallocate and shrunk on resize
    • Access is now synchronized with resize / delete operations
    • Better lifetime management with the renderparam through BPrim::Finalize
  • Picking and viewport overlays are also working now by slightly relaxing GetSourceName

(edit: Rethinking about it, this might not be an issue at all but indicate that the time to first pixel is lower)
One Issue is that on extremely simple scenes you can notice that the first frame displayed is lower resolution than typically. My intuition is that it is because blitting to the HdRenderBuffer happens exclusively with rendering and at the same time requires locking access to the Cycles Render Buffers. This hypothesis is (just partly) sustained by one performance metric. The new blitting code path is substantially faster than the one going through the device tasks, even if not fully optimized (the old code path only blitted one):

Function Time
copy_to_display_buffer (Old) 0.03 +- 0.001 s
on_display_cb (New) 0.001 +- 0.0005 s

The tiled and adaptive code path could be further unified together, this is a first step.

Houdini grid overlay
image

Picking
image

Viewport AOVs
image

@dedoardo dedoardo added the enhancement New feature or request label May 4, 2021
@dedoardo dedoardo self-assigned this May 4, 2021
@dedoardo dedoardo marked this pull request as ready for review May 17, 2021 18:26
@dedoardo dedoardo requested a review from Vochsel as a code owner May 17, 2021 18:26
@dedoardo
Copy link
Contributor Author

dedoardo commented May 17, 2021

Thinking about it, the fact that we see a lower resolution frame earlier should mean that it's more responsive.
todo: measure

…meraDepth, with a temp fallback for old scenes to be compatible (this fixes default viewports to have correct depth information and the camera tumble to work correctly)

- Removed HdAovTokens->depth from ccl::PASS_MIST mapping, now it uses a custom HdCyclesAovTokens->Mist token
@boberfly
Copy link
Contributor

Doing a stress-test here @dedoardo and adding/removing a bunch of AOVs seems to work great, no crashes. It does get slower the more AOVs you load in but that's definitely to be expected, it feels fast enough with Houdini's default color+depth+normal loaded in though and we can be mindful of making custom viewport settings which don't load all AOVs at once in the default case...

My little patch tweak uses ccl::PASS_DEPTH instead of ccl::PASS_MIST and we discovered it wasn't quite what Hydra expects and has a weird default of the depth value set to 100.0f instead of 1.0f (which threw off the camera tumble code in the viewport, which must use depth for camera calculations).

Awesome work @dedoardo !

@boberfly
Copy link
Contributor

Just an update, the depth seems to work on windows but I am not sure why linux doesn't seem to work quite right yet...

Copy link
Collaborator

@skwerner skwerner left a comment

Choose a reason for hiding this comment

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

Can you run clang-format on this patch?

plugin/hdCycles/renderBuffer.cpp Outdated Show resolved Hide resolved
plugin/hdCycles/renderBuffer.cpp Outdated Show resolved Hide resolved
plugin/hdCycles/renderBuffer.cpp Outdated Show resolved Hide resolved
@dedoardo
Copy link
Contributor Author

Thanks for reviewing @skwerner, ran clang-format and changed the scoped lock.

@boberfly
Copy link
Contributor

boberfly commented May 19, 2021

Hey I've got a screenshot of the latest patch here side by side windows vs linux, not sure why the grid doesn't get culled on the linux side (visually the AOVs are being made when I toggle depth/normal so they do exist and are working):
depth_windows
depth_linux

@dedoardo
Copy link
Contributor Author

After the discussion today I am merging this for now and keeping #111 alive, making it linux specific

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

Successfully merging this pull request may close these issues.

4 participants