Fix #123324: Improve Cycles camera bounding box size calculation #123341

Alaska · 2024-06-18T01:50:43+02:00

Alaska commented

2024-06-18 01:50:43 +02:00

Cycles runs a check to see if the camera is possibly inside a
volumetric object by seeing if the bounding box of the camera
and volumetric object intersect.

If the calculation is wrong (Cycles says the camera is outside the
volume when it's inside it), then the volume will not render properly.

This commit resolves most of these issues by making the camera
bounding box larger than before, taking into consideration
features like:

The impact DOF could have on the camera ray start position and how
that should impact the bounding box size.
Taking into consideration near clipping, which was missed from the
orthographic camera due to a oversight in a previous commit
(08cc73a9bb).

Cycles runs a check to see if the camera is possibly inside a volumetric object by seeing if the bounding box of the camera and volumetric object intersect. If the calculation is wrong (Cycles says the camera is outside the volume when it's inside it), then the volume will not render properly. This commit resolves most of these issues by making the camera bounding box larger than before, taking into consideration features like: 1. The impact DOF could have on the camera ray start position and how that should impact the bounding box size. 2. Taking into consideration near clipping, which was missed from the orthographic camera due to a oversight in a previous commit (08cc73a9bb).

Alaska added 1 commit 2024-06-18 01:50:46 +02:00

Fix 123324: Incorrect involume check on orthographic cameras 8d40529c47

Alaska changed target branch from main to blender-v4.2-release

2024-06-18 01:51:01 +02:00

Alaska requested review from Brecht Van Lommel 2024-06-18 01:51:09 +02:00

Alaska requested review from Sergey Sharybin 2024-06-18 01:51:21 +02:00

Alaska requested review from Lukas Stockner 2024-06-18 01:51:21 +02:00

Alaska commented

2024-06-18 01:57:32 +02:00

There are still cases where it can be wrong. Specifically when depth of field is in use, and there is a volume object which a DOF ray starts in, but the volume object does not intersect with the orthographic camera bounding view. This case is now more common after 08cc73a9bb .

A fix for this is listed as a TODO: /* TODO(sergey): Aperture support? */

There are still cases where it can be wrong. Specifically when depth of field is in use, and there is a volume object which a DOF ray starts in, but the volume object does not intersect with the orthographic camera bounding view. This case is now more common after 08cc73a9bbbddb714805f0977ec2cb47f556573f . A fix for this is listed as a TODO: `/* TODO(sergey): Aperture support? */`

Alaska commented

2024-06-18 02:35:56 +02:00

An alternative fix can be found here: !123343
If you would prefer that pull request, just say so.

The same issue with some volumetric objects not rendering properly with DOF also applies there.

An alternative fix can be found here: !123343 If you would prefer that pull request, just say so. The same issue with some volumetric objects not rendering properly with DOF also applies there.

Brecht Van Lommel commented

2024-06-18 12:39:39 +02:00

I think the fix should be in Camera::viewplane_bounds_get. We can make the bounds bigger to account for the near clipping and aperture. Something like this, but I have not tested or checked the math closely. And maybe the panorama case needs work too.

diff --git a/intern/cycles/scene/camera.cpp b/intern/cycles/scene/camera.cpp
index e53eafb..966891f 100644
--- a/intern/cycles/scene/camera.cpp
+++ b/intern/cycles/scene/camera.cpp
@@ -618,15 +618,17 @@ BoundBox Camera::viewplane_bounds_get()
     }
   }
   else {
-    bounds.grow(transform_raster_to_world(0.0f, 0.0f));
-    bounds.grow(transform_raster_to_world(0.0f, (float)height));
-    bounds.grow(transform_raster_to_world((float)width, (float)height));
-    bounds.grow(transform_raster_to_world((float)width, 0.0f));
+    const float extend = aperturesize + nearclip;
+
+    bounds.grow(transform_raster_to_world(0.0f, 0.0f), extend);
+    bounds.grow(transform_raster_to_world(0.0f, (float)height), extend);
+    bounds.grow(transform_raster_to_world((float)width, (float)height), extend);
+    bounds.grow(transform_raster_to_world((float)width, 0.0f), extend);
     if (camera_type == CAMERA_PERSPECTIVE) {
       /* Center point has the most distance in local Z axis,
        * use it to construct bounding box/
        */
-      bounds.grow(transform_raster_to_world(0.5f * width, 0.5f * height));
+      bounds.grow(transform_raster_to_world(0.5f * width, 0.5f * height), extend);
     }
   }
   return bounds;

There might be more exact ways to do it, but it doesn't matter much if the bounds are a bit bigger or smaller, it's just to avoid the volume check in the most common cases.

I think the fix should be in `Camera::viewplane_bounds_get`. We can make the bounds bigger to account for the near clipping and aperture. Something like this, but I have not tested or checked the math closely. And maybe the panorama case needs work too. ```Diff diff --git a/intern/cycles/scene/camera.cpp b/intern/cycles/scene/camera.cpp index e53eafb..966891f 100644 --- a/intern/cycles/scene/camera.cpp +++ b/intern/cycles/scene/camera.cpp @@ -618,15 +618,17 @@ BoundBox Camera::viewplane_bounds_get() } } else { - bounds.grow(transform_raster_to_world(0.0f, 0.0f)); - bounds.grow(transform_raster_to_world(0.0f, (float)height)); - bounds.grow(transform_raster_to_world((float)width, (float)height)); - bounds.grow(transform_raster_to_world((float)width, 0.0f)); + const float extend = aperturesize + nearclip; + + bounds.grow(transform_raster_to_world(0.0f, 0.0f), extend); + bounds.grow(transform_raster_to_world(0.0f, (float)height), extend); + bounds.grow(transform_raster_to_world((float)width, (float)height), extend); + bounds.grow(transform_raster_to_world((float)width, 0.0f), extend); if (camera_type == CAMERA_PERSPECTIVE) { /* Center point has the most distance in local Z axis, * use it to construct bounding box/ */ - bounds.grow(transform_raster_to_world(0.5f * width, 0.5f * height)); + bounds.grow(transform_raster_to_world(0.5f * width, 0.5f * height), extend); } } return bounds; ``` There might be more exact ways to do it, but it doesn't matter much if the bounds are a bit bigger or smaller, it's just to avoid the volume check in the most common cases.

Alaska added 2 commits 2024-06-18 14:31:53 +02:00

Revert "Fix 123324: Incorrect involume check on orthographic cameras" 3da4747610

This reverts commit 8d40529c47.

Extend BBox based on aperture c5a4bfa221

Alaska commented

2024-06-18 14:34:08 +02:00

I've updated the pull request with a variant of what @brecht recommended.

~~My concern is that this configuration may not work perfectly for cameras that aren't aligned with the world axis. But I haven't investigated it yet.~~

In theory this isn't a issue. But I have been wrong before.

I've updated the pull request with a variant of what @brecht recommended. ~~My concern is that this configuration may not work perfectly for cameras that aren't aligned with the world axis. But I haven't investigated it yet.~~ In theory this isn't a issue. But I have been wrong before.

Alaska changed title from ~~Fix #123324: Incorrect camera bbox for involume check of orthographic camera~~ to WIP: Fix #123324: Incorrect camera bbox for involume check of orthographic camera

2024-06-18 14:36:39 +02:00

Alaska added 1 commit 2024-06-18 14:42:42 +02:00

Fix NaN when DOF is disabled b1ecea2be8

Alaska changed title from ~~WIP: Fix #123324: Incorrect camera bbox for involume check of orthographic camera~~ to Fix #123324: Incorrect camera BBox for involume check of orthographic camera

2024-06-18 15:03:15 +02:00

Alaska added 1 commit 2024-06-18 15:06:15 +02:00

Remove comupation on float2 69a3397f18

Brecht Van Lommel commented

2024-06-18 15:09:40 +02:00

Thanks!

For the panorama case, I think we also want to replace nearclip by max_aperture_size + nearclip.

Thanks! For the panorama case, I think we also want to replace `nearclip` by `max_aperture_size + nearclip`.

Alaska reviewed 2024-06-18 15:12:52 +02:00

intern/cycles/scene/camera.cpp Outdated

						
				@ -625,0 +627,4 @@

				     scaled value is larger than nearclip, in which case we add it to `extend` to extend the

				     bounding box to account for these rays.

				     ----------------- nearclip plane

Alaska commented

2024-06-18 15:12:52 +02:00

This diagram and the corrisponding code matches the orhtographic implementation of DOF with nearcliping.

Pespective cameras differ and technically should use a smaller extend value (just max_aperture_size + nearclip) because of how DOF with nearcliping is implemented there ray->P += nearclip * ray->D.

This diagram and the corrisponding code matches the orhtographic implementation of DOF with nearcliping. Pespective cameras differ and technically should use a smaller `extend` value (just `max_aperture_size + nearclip`) because of how DOF with nearcliping is implemented there `ray->P += nearclip * ray->D`.

Alaska added 1 commit 2024-06-18 15:19:57 +02:00

Adjust panoramic case 0dd7deefac

Alaska changed title from ~~Fix #123324: Incorrect camera BBox for involume check of orthographic camera~~ to Fix #123324: Improve Cycles camera bounding box size calculation

2024-06-18 15:26:21 +02:00

Brecht Van Lommel requested changes 2024-06-18 16:25:52 +02:00

Dismissed

Brecht Van Lommel left a comment

Only minor nitpicks about style.

intern/cycles/scene/camera.cpp

						
				@ -593,40 +593,73 @@ BoundBox Camera::viewplane_bounds_get()

				   * checks we need in a more clear and smart fashion? */

				  BoundBox bounds = BoundBox::empty;

				  float extend;

Brecht Van Lommel commented

2024-06-18 16:21:57 +02:00

It's better to use const float extend = twice than declaring in advance. Less chance of using uninitialized variables.

It's better to use `const float extend = ` twice than declaring in advance. Less chance of using uninitialized variables.

Alaska marked this conversation as resolved

intern/cycles/scene/camera.cpp Outdated

						
				@ -623,2 +626,2 @@

				    bounds.grow(transform_raster_to_world((float)width, (float)height));

				    bounds.grow(transform_raster_to_world((float)width, 0.0f));

				    /* max_aperture_size = Max horizontal distance a ray travels from aperture edge to focus point.

				     Scale that value based on the ratio between focaldistance and nearclip to figure out the

Brecht Van Lommel commented

2024-06-18 16:23:45 +02:00

Multiline comment style is:

/* abc
 * def
 * ghi
 */

Multiline comment style is: ``` /* abc * def * ghi */ ```

Alaska marked this conversation as resolved

intern/cycles/scene/camera.cpp Outdated

						
				@ -625,0 +647,4 @@

				    float scaled_horz_dof_ray = 0.0f;

				    if (max_aperture_size > 0.0f) {

				      scaled_horz_dof_ray = max_aperture_size * (nearclip / focaldistance);

				    }

Brecht Van Lommel commented

2024-06-18 16:25:10 +02:00

Prefer single initialization:

const float scaled_horz_dof_ray = (max_aperture_size > 0.0f)  ?
  max_aperture_size * (nearclip / focaldistance) : 0.0f;

Prefer single initialization: ``` const float scaled_horz_dof_ray = (max_aperture_size > 0.0f) ? max_aperture_size * (nearclip / focaldistance) : 0.0f; ```

Alaska marked this conversation as resolved

Alaska added 1 commit 2024-06-18 16:31:33 +02:00

Address Brechts review

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.