Arnold improvements #1775
Merged
Arnold improvements #1775
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
This is motivated by a need to support attributes that affect Arnold's tesselation settings on polymesh objects, as well as a desire to reintroduce automatic instancing capabilities into the renderer backends. In an ideal world, attributes and objects would be truly orthogonal, and the unique hash for an object undergoing automatic instancing would simply be the hash of the object. For the Arnold backend, that is unfortunately not the case since various attributes will need to be taken into account in generating the instancing hash. This implies that attributes are needed at the point the object is converted to the renderer, presenting us with several implementation options : 1. Require an initial set of attributes to be passed to `Renderer::object()`, with only subsequent edits using `ObjectInterface::attributes()`. 2. Buffer the IECore::Object internally in the renderer, until the attributes are provided by `ObjectInterface::attributes()` and it can be converted. Mandate that client code _must_ call `Object::Interface::attributes()` so that we have this opportunity. 3. Add an `ObjectInterface::flush()` method which makes the synchronisation step from 2) more explicit. Since it's impossible to enforce correct client behaviour for 2) and 3), we have implemented 1) here in this commit. It is a pity that initial attribute specification is not done through the same method as subsequent edits, but in practice (see unit tests) it's actually pretty convenient to be able to specify the attributes inline with the object creation.
This is actually a pretty big refactoring, since it also tidies up some preexisting ugliness : - The separate SceneGraph updateAttributes()/updateObject() public functions were forcing us into a situation where we needed a public finalise() method to resolve all the edits as a final step. This was going to get even messier, so we now have a single public update() method in SceneGraph which gives it enough control to tidily deal with the new coupling of object and attribute creation. - The finalise() method was requiring us to store the m_pending state in SceneGraph. This was an annoyance since it was increasing the size of the SceneGraph, but didn't actually need to be persistent. It was also doing double duty as an ad-hoc means of querying parent changes via parentPending(), which was fragile and forced us to do pending changes on parents only after processing the children. Putting a single SceneGraph::update() method in charge allows us to store this state temporarily, and track parent changes explicitly. - The global attributes were being dealt with as a special case in the InteractiveRender node, but are now dealt with as a regular part of the SceneGraph update. This puts us in a much better place for future changes, which is important because to be able to support edits to arnold subdivision and displacement attributes, we're going to have to introduce the new concept of a failed attribute edit that triggers recreation of the object.
Arnold 4.2.8.0 introduced the subdiv_adaptive_space parameter and renamed subdiv_pixel_error to subdiv_adaptive_error.
Where two or more objects are identical (share the same hash), we now automatically create them as instances of the same underlying node in Arnold. The only exception to this is where the subdivision settings for a polymesh are view dependent, in which case we cannot expect a single master mesh to be subdivided appropriately for all instances - in this case we automatically disable the instancing.
Attributes prefixed with "user:" are transferred into Arnold as user parameters on AtNodes, available with `AiUserGet*()` methods in shaders, and directly to the user via shaders such as userDataColor, userDataFloat etc.
| /// \todo Rejig class hierarchy so we can have something less generic than | ||
| /// Object here, but still pass CoordinateSystems. Or should | ||
| /// coordinate systems have their own dedicated calls? | ||
| virtual ObjectInterfacePtr object( const std::string &name, const IECore::Object *object ) = 0; | ||
| virtual ObjectInterfacePtr object( const std::string &name, const IECore::Object *object, const AttributesInterface *attributes ) = 0; |
There was a problem hiding this comment.
Should the new arg have a null default? It seems a bit annoying that you have to send an empty CompoundObject in the tests.
There was a problem hiding this comment.
I did consider that, but decided against it. I wanted to make it absolutely clear that it was necessary to provide the initial set of attributes (rather than subsequent edits) upfront and not after creation, and providing a default argument would make it possible to get that wrong.
|
Looks like the tests failed with a couple casting issues |
|
I've pushed a fix for the casting issue. |
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.
This adds :
Currently the InteractiveArnoldRender doesn't update correctly on changes to the subdivision attributes - either tweaking the mesh geometry directly or restarting the render is necessary to get those to come through. This is basically an Arnold limitation, but I do have a plan for addressing it soon on the Gaffer side. Since this is already a fairly complex PR, I'd like to address that as part of the upcoming displacement support rather than add more complexity here.
I feel sure that this PR will conflict with the mesh light PR that is still awaiting review. For what it's worth, I think it'd be easier to fix up that one after merging this...