Pytorch3d: Rasterizer shouldn't require cameras as positional parameters

[yifita]

🚀 Feature

Motivation

In most modules, you did a great job being flexible about the inputs during initialization, so that at forward time, these inputs can be provided additionally.

An example is
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/shader.py#L45
and
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/shader.py#L56

An exception is the rasterizer, both for points and mesh. Somehow cameras becomes a required input upon initialization.
This is inconsistent with the remaining style and also not flexible.

Pitch

Basically use the same way to initialize rasterizers as the other classes, such as the shader.

[gkioxari]

Hi @yifita
cameras should be a required input for rasterization as the camera properties (position, type of camera, etc.) play an important role in rasterization. So I fail to understand how cameras can be omitted. You provide the example of lights but cameras and lights don't hold the same bearing in a rendering pipeline.

[jcjohnson]

Hi @yifita, let me try to rephrase to make sure I understand:

For most rendering settings, we allow them to be either specified in the initializer, or overridden in the forward pass. This was a deliberate design choice on our end, since it allows you to be more flexible about which parameters are learned vs which are fixed (which may vary for different applications). If you want to learn a parameter (e.g. light color or position), either by optimizing it directly or having its parameters output by an auxiliary network, then you need to be able to pass that parameter through the rendering pipeline on each forward pass. However if you want to fix some parameter and not learn it, then forcing the user to pass it on each forward pass is annoying, since they have to track this external state themselves; in this use case it's more convenient to fix the parameter in the initializer.

However for the camera we don't follow this convention -- we force it to be specified on the forward pass, and don't allow it to be fixed during initialization.

The reason that cameras are treated differently from other parameters is that the camera might be needed by both the rasterizer (to transform meshes from world to screen space) and by the compositor (for lighting computations). The point cloud compositors we have currently implemented don't use cameras, but some of our mesh shaders do -- and we wanted the mesh and point cloud renderers to follow the same design. If we allowed cameras to be specified during initialization, then there is a concern that the user might accidentally specify different cameras for the rasterizer and compositor, which would lead to hard-to-debug errors.

However I do agree that it would be nice to allow cameras to be fixed during initialization, to be consistent with other parameters. In order to prevent the possible user error above, the renderer initializer should check that both the rasterizer and compositor have a camera specified, and they both are the same object. We would then want to do the same for both the MeshRenderer and the PointsRenderer.

[yifita]

Hi all,

thanks for your replies.

However for the camera we don't follow this convention -- we force it to be specified on the forward pass, and don't allow it to be fixed during initialization.

@jcjohnson maybe i didn't get it, but actually, the code is opposite to what you said. The rasterizers force the user to provide cameras at initiation, instead of at forward pass.

https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/rasterizer.py#L57

[jcjohnson]

Ooops, my mistake for not actually carefully re-reading the code and just going on memory. To be more complete:

Mesh rasterizer

The mesh rasterizer expects a camera in the initializer:
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/rasterizer.py#L57

But it can be overridden via kwargs in the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/rasterizer.py#L90

Mesh shader

Mesh shaders also generally expect cameras in their initializers:
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/shader.py#L43

But these can also be overridden via kwargs in the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/shader.py#L55

Mesh renderer

The mesh renderer passes kwargs along to both the rasterizer and shader during the forward pass, so cameras can be override to both via kwargs during the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/686c8666d31d932ed42d3cd7319f249fc75e89a9/pytorch3d/renderer/mesh/renderer.py#L39

Point rasterizer

The point rasterizer is the same as the mesh rasterizer: it expects cameras in the initializer:
https://github.com/facebookresearch/pytorch3d/blob/master/pytorch3d/renderer/points/rasterizer.py#L51

But they can be overridden via kwargs in the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/master/pytorch3d/renderer/points/rasterizer.py#L82

Point compositors

None of our current point compositors use cameras at all. So they don't appear in either the initializer or the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/master/pytorch3d/renderer/points/compositor.py#L19

Point renderer

The point renderer also passes its kwargs along to both the rasterizer and the compositor in the forward pass:
https://github.com/facebookresearch/pytorch3d/blob/master/pytorch3d/renderer/points/renderer.py#L37

[jcjohnson]

So what this means is that we already have flexibility for providing cameras at initialization, or overriding them at each forward pass. The only missing piece is that the rasterizer initializers expect cameras as a positional argument, so if you intend to override them in the forward pass you are forced to pass something for this positional cameras argument, which feels awkward. As a workaround for this use case, you can pass None as the cameras parameter in the rasterizer initializers, make sure to pass cameras via kwargs on the renderer forward pass, and everything should work.

Probably the best way to resolve this moving forward is to make cameras an optional argument (with default None) in the initializers of both rasterizers; then in the forward pass throw an exception if no camera was passed via the initializer or the kwargs of the forward pass.

[yifita]

So what this means is that we already have flexibility for providing cameras at initialization, or overriding them at each forward pass. The only missing piece is that the rasterizer initializers expect cameras as a positional argument in their initializers, so if you intend to override them in the forward pass you are forced to pass _something_ for this positional cameras argument, which feels awkward.

Exactly :smile:
But I also see the problem you mentioned about having different cameras in rasterizer and compositor. I didn't think about that.

Probably the best way to resolve this is to make cameras an optional argument (with default None) in the initializers of both rasterizers; then in the forward pass throw an exception if no camera was passed via the initializer or the kwargs of the forward pass.

This is definitely a solution.

[jcjohnson]

This should now be fixed in https://github.com/facebookresearch/pytorch3d/commit/17ca6ecd81b49751fd4adcbd6effee9f3fbfc455. If that solves the problem, feel free to close the issue!