Platform review of server

Author: jwnimmer-tri

CONTRIBUTING.md

@@ -5,4 +5,4 @@
 ## Terms and Conditions
 
 When you make a contribution to this project, you are agreeing to offer your
-contribition under the same [LICENSE.TXT](LICENSE.TXT) as the project.
+contribution under the same [LICENSE.TXT](LICENSE.TXT) as the project.

README.md

@@ -15,14 +15,6 @@ Please share your issues and improvements on GitHub.**
 This software is only tested on Ubuntu 22.04 "Jammy", but should probably
 work with any Python interpreter that supports our `requirements.txt`.
 
-## Building and testing
-
-From a git checkout of `drake-blender`:
-
-```sh
-./bazel test //...
-```
-
 ## Running the render server
 
 From a git checkout of `drake-blender`:
@@ -39,6 +31,14 @@ are available in your Python runtime environment.
 
 See [examples](examples/README.md).
 
+## Testing (for developers)
+
+From a git checkout of `drake-blender`:
+
+```sh
+./bazel test //...
+```
+
 # Credits
 
 The Drake-Blender project was created by the

server.py

@@ -138,8 +138,9 @@ def render_image(self, *, params: RenderParams, output_path: Path):
                 exec(code, {"bpy": bpy}, dict())
 
         # Import a glTF file. Note that the Blender glTF importer imposes a
-        # 90-degree rotation along X-axis when loading meshes. Thus, we
+        # +90 degree rotation around the X-axis when loading meshes. Thus, we
         # counterbalance the rotation right after the glTF-loading.
+        # TODO(#39) This is very suspicious. Get to the bottom of it.
         bpy.ops.import_scene.gltf(filepath=str(params.scene))
         bpy.ops.transform.rotate(
             value=math.pi/2, orient_axis='X', orient_type='GLOBAL'
@@ -151,9 +152,8 @@ def render_image(self, *, params: RenderParams, output_path: Path):
         scene.render.filepath = str(output_path)
         scene.render.resolution_x = params.width
         scene.render.resolution_y = params.height
-        aspect_ratio = params.focal_y / params.focal_x
         scene.render.pixel_aspect_x = 1.0
-        scene.render.pixel_aspect_y = aspect_ratio
+        scene.render.pixel_aspect_y = params.focal_y / params.focal_x
 
         # Set camera parameters.
         camera = bpy.data.objects.get("Camera Node")
@@ -169,6 +169,10 @@ def render_image(self, *, params: RenderParams, output_path: Path):
         # Set the clipping planes using {min, max}_depth when rendering depth
         # images; otherwise, `near` and `far` are set for color and label
         # images.
+        # TODO(#38) This clipping logic fails to implement kTooClose.
+        # When there is geometry within [near,far] clip but outside [min,max]
+        # depth, we should return zero (too close) not maxint (too far). Fix
+        # the code here and add regression tests for both too-close and -far.
         camera.data.clip_start = (
             params.min_depth if params.min_depth else params.near
         )
@@ -181,17 +185,9 @@ def render_image(self, *, params: RenderParams, output_path: Path):
             params.center_y - 0.5 * params.height
         ) / params.width
 
-        # TODO(bassamul.haq/zachfang): Currently, `camera.data.angle` will not
-        # be set if the values of fov_x and fov_y are different. The code path
-        # needs to be tested.
-        if params.fov_x == params.fov_y:
-            camera.data.lens_unit = "FOV"
-            camera.data.angle = params.fov_x
-        else:
-            _logger.warning(
-                f"fov_x {params.fov_x} != fov_y {params.fov_y}. "
-                "'camera.data.angle' is not set."
-            )
+        camera.data.lens_unit = "FOV"
+        camera.data.angle_x = params.fov_x
+        camera.data.angle_y = params.fov_y
 
         # Set image_type specific functionality.
         if params.image_type == "color":
@@ -294,7 +290,7 @@ def create_depth_node_layer(self, min_depth=0.01, max_depth=10.0):
 
         # Convert depth measurements via a MapValueNode. The depth values are
         # measured in meters, and thus they are converted to millimeters first.
-        # Blender scales the pixel values by 65535 (2^16 -1) when producing an
+        # Blender scales the pixel values by 65535 (2^16 -1) when producing a
         # UINT16 image, so we need to offset that to get the correct UINT16
         # depth.
         assert (

test/server_test.py

@@ -182,7 +182,8 @@ def test_label_render(self):
         )
 
     # Test color and depth image rendering given a rgba and a textured mesh.
-    # Note: Rendering a label image is not applicable for any textured objects.
+    # (We do not check a label image because by definition an RPC for a label
+    # image will never contain any textured objects.)
     def test_texture_color_render(self):
         self._render_and_check(
             gltf_path="test/one_rgba_one_texture_boxes.gltf",
@@ -203,7 +204,11 @@ def test_consistency(self):
         """Tests the consistency of the render results from consecutive
         requests. Each image type is first rendered and compared with the
         ground truth images. A second image is then rendered and expected to be
-        pixel-identical as the first one.
+        pixel-identical as the first one. As with all things Drake, we expect
+        reproducible simulations, so if there is any randomness in the render
+        pipeline it's the responsibility of the server to configure it so that
+        the exact same RPC call produces the exact same image output no matter
+        how it's called or how many times it's called.
         """
         TestCase = namedtuple(
             "TestCase", ["image_type", "reference_image", "threshold"]
@@ -227,7 +232,7 @@ def test_consistency(self):
             returned_image_paths.append(first_image)
 
         for index, test_case in enumerate(test_cases):
-            second_image = self._render_and_check(
+            self._render_and_check(
                 gltf_path=DEFAULT_GLTF_FILE,
                 image_type=test_case.image_type,
                 reference_image_path=returned_image_paths[index],