Pixar batch renderer refactoring and performance improvements #577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… legacy viewport This creates a public utility function out of the _ToMFrameContextDisplayStyle() private, static function in PxrMayaHdShapeAdapter. A subsequent commit will remove the old function and replace its usage with the new public function.
…tus for VP 2.0 This change removes the private, static _ToMFrameContextDisplayStyle for PxrMayaHdShapeAdapter in favor of the newly added px_LegacyViewportUtils::GetMFrameContextDisplayStyle().
…es bounding box rendering
…only The current batch renderer API offers Draw() functions that draw shapes, bounding boxes, or both depending on the user data. The usage, however, is that only the PxrHdImagingShape invokes a draw for all of the shapes, and then individual shapes only need to draw their bounding boxes. This change adds independent bounding box-only methods that the shapes can use when they need to draw their bounding boxes. Subsequent commits will update MPxDrawOverrides and MPxSurfaceShapeUIs to use the appropriate draw method.
…or viewport draw This ensures that the bounding box is always available in case the viewport display style is changed and we don't end up executing another shape adapter Sync().
Since practically we will now always have a bounding box in GetMayaUserData(), we remove the early return that used to check for it.
…tus from displayStatus There are a handful of Maya displayStatus values that we consider "active", which means that Hydra should draw them including their wireframe. This reduces some duplicated code for determining whether the shape is active.
This field will provide an easy way to re-use the state determined during Sync(). Subsequent changes will make it so that only the displayStatus (and not the viewport-specific displayStyle) determine whether or not to use the wireframe.
…der params This boolean will soon be based on only the shape's display status and not any viewport's display style, so it can accurately be cached during calls to Sync() and re-used in between.
… when computing repr This change renames the shape adapter GetReprSelectorForDisplayState() function to GetReprSelectorForDisplayStyle(), makes it non-virtual, and only requires that the displayStyle be provided. It will query Maya for the shape's displayStatus itself. We also use the render params' useWireframe field to determine whether or not to use a repr that includes the wireframe, in addition to the displayStyle. Note that we are still erroneously using the displayStyle at Sync() time to decide whether or not to enable lighting. This can be addressed later but for now it only causes subtly incorrect visual results for wireframe display styles in certain cases.
…nderParams() Draw overrides for individual shapes will call the batch renderer's DrawBoundingBox() and pxrHdImagingShape's draw override will call Draw(), so we don't need the shape adapters to track (unreliably) whether to draw its shape and/or bounding box, especially since those can also vary depending on each viewport's displayStyle.
…hape draw callbacks Only the pxrHdImagingShape will actually invoke a Hydra draw to render shapes, so the only reason that individual shape draw callbacks will do any drawing is if they need to draw the bounding box for their shape.
With the pxrHdImagingShape draw override now the only one that calls Draw(), we don't need any bounding-box related drawing code in those methods anymore, since individual shape draw overrides are instead expected to call DrawBoundingBox() as necessary from their own draw callbacks.
Since we may not get another Sync() callback if the viewport display style is changed, we need to make sure that the wireframe color is always available from the render params. This may lead to slightly different shape adapter bucketing behavior, since the render params for shapes will pretty much always have a non-zero wireframe color now, but it shouldn't practically impact performance significantly. This also modifies the _GetWireframeColor() function to handle the MColor to GfVec4f conversion so that clients don't have to, and so that they can pass a pointer to the render params' wireframeColor directly.
…or define them inline
Derived classes can control their own behaviors in the protected _Sync().
This moves all computation of a unique identifier for the shape into the shape adapter base class and removes it from derived classes. Instead of a fixed string and a hash of the Maya object, we instead query Maya for the node's UUID and use that directly as the shape's "identifier", which we also use both as the name of its rprim collection, and to construct the delegate ID. We use the node's UUID since MPxDrawOverrides may be constructed and destructed over the course of a node's lifetime (e.g. by doing 'ogs -reset'). Since the draw override owns the shape adapter, we therefore need an identifier tied to the node's lifetime rather than to the shape adapter's lifetime. With the delegate ID now stored in the base class, its accessor no longer needs to be virtual, and also gets defined inline.
Now that a bunch of other shape adapter data is recomputed whenever _SetDagPath() is called, we want to make sure that _shapeDagPath is not changed any other way, so this change makes it private instead of protected. Existing usage of the field now uses the public accessor instead.
…one in scene delegate
This makes the ordering in the cpp file consistent with the declaration order in the header.
…b it down to GetRenderTasks() displayStatus may vary per viewport, so we need to select the repr for each shape at draw time rather than at Sync() time. Subsequent commits will make use of the displayStyle by querying each shape adapter for the repr that should be used.
…n GetRenderTasks()
…params hash Since the parameters to the selection task do not vary based on anything shape related and in fact never change at all, there's no reason that we need one per set of unique render params. We can just always use the same one for all shapes.
… hashes to render setup tasks This change simplifies the mapping of HdRprimCollection names to render task IDs and hashes of render params to render setup task IDs, respectively. Since the batch renderer now never changes the collection of a render task itself, it no longer needs to mark them dirty on every draw. Note that this commit would actually cause somewhat of a performance regression on its own, since the single render task per collection would get thrashed a bit more as its repr is changed. Subsequent commits will make the shape adapters manage a collection and a render task ID *per* repr. This also allows the shape adapter to notify the change tracker when its collections or tasks need to be dirtied.
…) using prim filters This will allow us to delay computing which repr to use until draw time and to query the necessary data from the shape adapters directly. The original mechanism will be supported by specifying the shape adapter as nullptr in the prim filter. This commit adds the plumbing and a subsequent commit will take advantage of it when the shape adapters manage a collection per repr.
…ask ID per repr This change provides a consistent one-to-one mapping between reprs, collections, and render tasks in the shape adapter. This results in minimal draw batch thrashing within hydra when the repr changes due to a change in Maya viewport displayStyle, since the repr on the collection never needs to change and the collection on the render task never needs to change. The shape adapter must be queried for which collection and render task ID to use based on a particular repr, so that handling has been added where necessary in the batch renderer and scene/task delegate. When rendering, the shape adapters are plumbed through to GetRenderTasks() where we determine what repr to use based on the displayStyle, and then use that repr to get the appropriate collection and render task ID for each shape we're drawing.
…GetRenderTasks() Storing whether or not to draw the shape during Sync() is not correct since we may be drawing for multiple viewports with differing displayStyles, or we may change displayStyles on a particular viewport and not get another prepareForDraw() callback. To fix this, instead of setting the delegate's root visibility during sync, we check during GetRenderTasks() to see whether there's an active render for the given displayStatus, and simply skip adding the render task if there isn't one.
…asks Though this is not yet being used, this function can be used to notify the change tracker when the shape adapter detects a change in the shape that requires regenerating draw batches.
…cpp files Symbols from these headers are used, but the headers were not being included.
kxl-adsk
approved these changes
Jun 10, 2020
| return iter->second; | ||
| } | ||
|
|
||
| static const SdfPath emptyTaskId; |
There was a problem hiding this comment.
Why return a reference from this method? It forces you to have a static empty task in case there is no render task ID.
There was a problem hiding this comment.
This was primarily a performance consideration to minimize copying, since GetRenderTaskId() is going to get called once per proxy shape for every frame draw.
Though now that I'm looking at this, I think I could have used return SdfPath::EmptyPath() here. :/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To buy us some time with our production artists while we tackle adopting the VP2.0 render delegate, I took one final pass at improving and optimizing the Pixar batch renderer.
The biggest benefit for them here is a fix for a long-standing issue where the displayStyle of the viewport was only being considered at
prepareForDraw()time. This caused draw issues when changing the style of a viewport where the display wouldn't update until anotherprepareForDraw()was issued (for example by performing a selection).There is some marginal performance improvement here as well though as a result of avoiding ever dirtying the rprim collection or the repr for a given render task. The shape adapters now manage a collection for each repr that might be used by the Maya viewport, and a render task ID for each of those collections. The batch renderer itself still instantiates the actual tasks lazily.
I tried to narrate the process as best I could in the commit history, but let me know if anything seems unclear.
And just to reiterate, I still wouldn't recommend that anyone introduce new usage of the Pixar batch renderer. We ourselves are aiming to adopt the VP2.0 render delegate over the next few months and don't intend to put any additional effort into improving the batch renderer. This pass was solely to provide some relief to production while we pursue that adoption.