Cycles : Simplify transform handling - #7136
Conversation
- Remove unreachable code. CameraAlgo never sets `motion_position`, so we don't need to handle the different cases. It's not even clear that the different cases need different handling - I suspect not. - Drop all the sample interpolation stuff. We learned we don't need it when refactoring the object deformation code. - Do scaling and conversion once, instead of repeating for `set_tfm()` and `set_motion()`. - Document weird scaling in Y, which I think should be removable in future.
As with object deformation, none of this is necessary now - Cycles happily accepts any number of samples.
b21bc23 to
7b7c159
Compare
murraystevenson
left a comment
murraystevenson
left a comment
There was a problem hiding this comment.
Thanks John, this looks like a great simplification overall. I've noted a pair of subdivision related wrinkles inline, one straightforward, the other a little vexing...
| if( mesh->get_subdivision_type() != ccl::Mesh::SUBDIVISION_NONE ) | ||
| { | ||
| // Subdivision meshes can't be auto-instanced, so | ||
| // we should be the only one managing `subd_objecttoworld`. | ||
| assert( m_geometry.use_count() == 1 ); |
There was a problem hiding this comment.
With the addition of object-space subdivision in Cycles 5.0, we do auto-instance those meshes, so I think this should also be gated on && mesh->get_subd_adaptive_space() == ccl::Mesh::SUBDIVISION_ADAPTIVE_SPACE_PIXEL as this assert does fire on a scene with duplicated spheres all set to object-space subdivision. Looks like I missed updating this as part of the Cycles 5.0 update...
| // Subdivision meshes can't be auto-instanced, so | ||
| // we should be the only one managing `subd_objecttoworld`. | ||
| assert( m_geometry.use_count() == 1 ); | ||
| mesh->set_subd_objecttoworld( m_object->get_tfm() ); |
There was a problem hiding this comment.
It appears the old code has an early-out when samples.size() == 1 and would never get here to call mesh->set_subd_objecttoworld() like we now do. As a result, transforming a pixel-space adaptive subdiv mesh with Cycles as the Viewer renderer now causes the mesh to disappear (I'm assuming because setting subd_objecttoworld is triggering an update to its adaptive subdivision, but Cycles is also expecting a fresh mesh to subdivide). So this new code is more correct, but less useful. Oy...
There was a problem hiding this comment.
This seems to have been down to a missing tag_update() call, which I also added in 9a4e0ea.
2 participants
I'm about to add PointInstancer support for Cycles, which will effectively mean making as many lightweight CyclesObjects as I can each with their own transform. Before doing that, let's simplify the transform handling in general, since it has a lot of cruft that doesn't seem necessary with modern Cycles.
History suggests that when I delete "redundant" code from GafferCycles I usually delete at least one thing that turned out to be necessary. But harking back to similar work for deformation blur, I've rendered a contact sheet showing various numbers of transform segments rendered across all devices and with both meshes and points - all seems to be well there :