From 7e55dfcf271dd1786baddbe35421b470250fb054 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Fri, 7 Jul 2023 20:15:36 +0200 Subject: [PATCH] Fix #103918: Cycles point cloud motion blur artifacts on the GPU Change storage to consistently put xyz + radius in the motion blur attribute. Pull Request: https://projects.blender.org/blender/blender/pulls/109830 --- intern/cycles/blender/pointcloud.cpp | 8 ++-- intern/cycles/bvh/build.cpp | 6 +-- intern/cycles/bvh/embree.cpp | 25 ++++------- intern/cycles/device/hiprt/device_impl.cpp | 6 +-- intern/cycles/device/metal/bvh.mm | 33 +++++++++----- intern/cycles/device/optix/device_impl.cpp | 50 ++++++++++++++-------- intern/cycles/scene/pointcloud.cpp | 17 +++----- intern/cycles/scene/pointcloud.h | 4 +- 8 files changed, 84 insertions(+), 65 deletions(-) diff --git a/intern/cycles/blender/pointcloud.cpp b/intern/cycles/blender/pointcloud.cpp index 40203a92630..34a92723985 100644 --- a/intern/cycles/blender/pointcloud.cpp +++ b/intern/cycles/blender/pointcloud.cpp @@ -31,7 +31,8 @@ static void attr_create_motion(PointCloud *pointcloud, const int num_points = pointcloud->get_points().size(); /* Find or add attribute */ - float3 *P = &pointcloud->get_points()[0]; + float3 *P = pointcloud->get_points().data(); + float *radius = pointcloud->get_radius().data(); Attribute *attr_mP = pointcloud->attributes.find(ATTR_STD_MOTION_VERTEX_POSITION); if (!attr_mP) { @@ -42,10 +43,11 @@ static void attr_create_motion(PointCloud *pointcloud, float motion_times[2] = {-1.0f, 1.0f}; for (int step = 0; step < 2; step++) { const float relative_time = motion_times[step] * 0.5f * motion_scale; - float3 *mP = attr_mP->data_float3() + step * num_points; + float4 *mP = attr_mP->data_float4() + step * num_points; for (int i = 0; i < num_points; i++) { - mP[i] = P[i] + get_float3(b_vector_attribute.data[i].vector()) * relative_time; + float3 Pi = P[i] + get_float3(b_vector_attribute.data[i].vector()) * relative_time; + mP[i] = make_float4(Pi.x, Pi.y, Pi.z, radius[i]); } } } diff --git a/intern/cycles/bvh/build.cpp b/intern/cycles/bvh/build.cpp index 8f175573e11..ddfc451d892 100644 --- a/intern/cycles/bvh/build.cpp +++ b/intern/cycles/bvh/build.cpp @@ -271,7 +271,7 @@ void BVHBuild::add_reference_points(BoundBox &root, const float3 *points_data = &pointcloud->points[0]; const float *radius_data = &pointcloud->radius[0]; const size_t num_points = pointcloud->num_points(); - const float3 *motion_data = (point_attr_mP) ? point_attr_mP->data_float3() : NULL; + const float4 *motion_data = (point_attr_mP) ? point_attr_mP->data_float4() : NULL; const size_t num_steps = pointcloud->get_motion_steps(); if (point_attr_mP == NULL) { @@ -298,7 +298,7 @@ void BVHBuild::add_reference_points(BoundBox &root, BoundBox bounds = BoundBox::empty; point.bounds_grow(points_data, radius_data, bounds); for (size_t step = 0; step < num_steps - 1; step++) { - point.bounds_grow(motion_data + step * num_points, radius_data, bounds); + point.bounds_grow(motion_data[step * num_points + j], bounds); } if (bounds.valid()) { references.push_back(BVHReference(bounds, j, i, PRIMITIVE_MOTION_POINT)); @@ -318,7 +318,7 @@ void BVHBuild::add_reference_points(BoundBox &root, for (uint j = 0; j < num_points; j++) { const PointCloud::Point point = pointcloud->get_point(j); const size_t num_steps = pointcloud->get_motion_steps(); - const float3 *point_steps = point_attr_mP->data_float3(); + const float4 *point_steps = point_attr_mP->data_float4(); /* Calculate bounding box of the previous time step. * Will be reused later to avoid duplicated work on diff --git a/intern/cycles/bvh/embree.cpp b/intern/cycles/bvh/embree.cpp index e894873ad98..aad985d8624 100644 --- a/intern/cycles/bvh/embree.cpp +++ b/intern/cycles/bvh/embree.cpp @@ -471,20 +471,6 @@ void BVHEmbree::set_curve_vertex_buffer(RTCGeometry geom_id, const Hair *hair, c } } -/** - * Pack the motion points into a float4 as [x y z radius] - */ -template -void pack_motion_points(size_t num_points, const T *verts, const float *radius, float4 *rtc_verts) -{ - for (size_t j = 0; j < num_points; ++j) { - rtc_verts[j].x = verts[j].x; - rtc_verts[j].y = verts[j].y; - rtc_verts[j].z = verts[j].z; - rtc_verts[j].w = radius[j]; - } -} - void BVHEmbree::set_point_vertex_buffer(RTCGeometry geom_id, const PointCloud *pointcloud, const bool update) @@ -520,13 +506,20 @@ void BVHEmbree::set_point_vertex_buffer(RTCGeometry geom_id, assert(rtc_verts); if (rtc_verts) { if (t == t_mid || attr_mP == NULL) { + /* Pack the motion points into a float4 as [x y z radius]. */ const float3 *verts = pointcloud->get_points().data(); - pack_motion_points(num_points, verts, radius, rtc_verts); + for (size_t j = 0; j < num_points; ++j) { + rtc_verts[j].x = verts[j].x; + rtc_verts[j].y = verts[j].y; + rtc_verts[j].z = verts[j].z; + rtc_verts[j].w = radius[j]; + } } else { + /* Motion blur is already packed as [x y z radius]. */ int t_ = (t > t_mid) ? (t - 1) : t; const float4 *verts = &attr_mP->data_float4()[t_ * num_points]; - pack_motion_points(num_points, verts, radius, rtc_verts); + memcpy(rtc_verts, verts, sizeof(float4) * num_points); } } diff --git a/intern/cycles/device/hiprt/device_impl.cpp b/intern/cycles/device/hiprt/device_impl.cpp index bbdb7f40355..f805359ae6d 100644 --- a/intern/cycles/device/hiprt/device_impl.cpp +++ b/intern/cycles/device/hiprt/device_impl.cpp @@ -618,7 +618,7 @@ hiprtGeometryBuildInput HIPRTDevice::prepare_point_blas(BVHHIPRT *bvh, PointClou const float3 *points_data = pointcloud->get_points().data(); const float *radius_data = pointcloud->get_radius().data(); const size_t num_points = pointcloud->num_points(); - const float3 *motion_data = (point_attr_mP) ? point_attr_mP->data_float3() : NULL; + const float4 *motion_data = (point_attr_mP) ? point_attr_mP->data_float4() : NULL; const size_t num_steps = pointcloud->get_motion_steps(); int num_bounds = 0; @@ -646,7 +646,7 @@ hiprtGeometryBuildInput HIPRTDevice::prepare_point_blas(BVHHIPRT *bvh, PointClou BoundBox bounds = BoundBox::empty; point.bounds_grow(points_data, radius_data, bounds); for (size_t step = 0; step < num_steps - 1; step++) { - point.bounds_grow(motion_data + step * num_points, radius_data, bounds); + point.bounds_grow(motion_data[step * num_points + j], bounds); } if (bounds.valid()) { bvh->custom_primitive_bound[num_bounds] = bounds; @@ -666,7 +666,7 @@ hiprtGeometryBuildInput HIPRTDevice::prepare_point_blas(BVHHIPRT *bvh, PointClou for (uint j = 0; j < num_points; j++) { const PointCloud::Point point = pointcloud->get_point(j); const size_t num_steps = pointcloud->get_motion_steps(); - const float3 *point_steps = point_attr_mP->data_float3(); + const float4 *point_steps = point_attr_mP->data_float4(); float4 prev_key = point.motion_key( points_data, radius_data, point_steps, num_points, num_steps, 0.0f, j); diff --git a/intern/cycles/device/metal/bvh.mm b/intern/cycles/device/metal/bvh.mm index 23560575b7f..267d8ab9ee5 100644 --- a/intern/cycles/device/metal/bvh.mm +++ b/intern/cycles/device/metal/bvh.mm @@ -516,20 +516,31 @@ bool BVHMetal::build_BLAS_pointcloud(Progress &progress, /* Get AABBs for each motion step */ size_t center_step = (num_motion_steps - 1) / 2; for (size_t step = 0; step < num_motion_steps; ++step) { - /* The center step for motion vertices is not stored in the attribute */ - if (step != center_step) { - size_t attr_offset = (step > center_step) ? step - 1 : step; - points = motion_keys->data_float3() + attr_offset * num_points; + if (step == center_step) { + /* The center step for motion vertices is not stored in the attribute */ + for (size_t j = 0; j < num_points; ++j) { + const PointCloud::Point point = pointcloud->get_point(j); + BoundBox bounds = BoundBox::empty; + point.bounds_grow(points, radius, bounds); + + const size_t index = step * num_points + j; + aabb_data[index].min = (MTLPackedFloat3 &)bounds.min; + aabb_data[index].max = (MTLPackedFloat3 &)bounds.max; + } } + else { + size_t attr_offset = (step > center_step) ? step - 1 : step; + float4 *motion_points = motion_keys->data_float4() + attr_offset * num_points; - for (size_t j = 0; j < num_points; ++j) { - const PointCloud::Point point = pointcloud->get_point(j); - BoundBox bounds = BoundBox::empty; - point.bounds_grow(points, radius, bounds); + for (size_t j = 0; j < num_points; ++j) { + const PointCloud::Point point = pointcloud->get_point(j); + BoundBox bounds = BoundBox::empty; + point.bounds_grow(motion_points[j], bounds); - const size_t index = step * num_points + j; - aabb_data[index].min = (MTLPackedFloat3 &)bounds.min; - aabb_data[index].max = (MTLPackedFloat3 &)bounds.max; + const size_t index = step * num_points + j; + aabb_data[index].min = (MTLPackedFloat3 &)bounds.min; + aabb_data[index].max = (MTLPackedFloat3 &)bounds.max; + } } } diff --git a/intern/cycles/device/optix/device_impl.cpp b/intern/cycles/device/optix/device_impl.cpp index 7426e99a415..97ef030f44b 100644 --- a/intern/cycles/device/optix/device_impl.cpp +++ b/intern/cycles/device/optix/device_impl.cpp @@ -1395,27 +1395,43 @@ void OptiXDevice::build_bvh(BVH *bvh, Progress &progress, bool refit) /* Get AABBs for each motion step. */ for (size_t step = 0; step < num_motion_steps; ++step) { /* The center step for motion vertices is not stored in the attribute. */ - const float3 *points = pointcloud->get_points().data(); - const float *radius = pointcloud->get_radius().data(); size_t center_step = (num_motion_steps - 1) / 2; - if (step != center_step) { - size_t attr_offset = (step > center_step) ? step - 1 : step; - /* Technically this is a float4 array, but sizeof(float3) == sizeof(float4). */ - points = motion_points->data_float3() + attr_offset * num_points; + + if (step == center_step) { + const float3 *points = pointcloud->get_points().data(); + const float *radius = pointcloud->get_radius().data(); + + for (size_t i = 0; i < num_points; ++i) { + const PointCloud::Point point = pointcloud->get_point(i); + BoundBox bounds = BoundBox::empty; + point.bounds_grow(points, radius, bounds); + + const size_t index = step * num_points + i; + aabb_data[index].minX = bounds.min.x; + aabb_data[index].minY = bounds.min.y; + aabb_data[index].minZ = bounds.min.z; + aabb_data[index].maxX = bounds.max.x; + aabb_data[index].maxY = bounds.max.y; + aabb_data[index].maxZ = bounds.max.z; + } } + else { + size_t attr_offset = (step > center_step) ? step - 1 : step; + const float4 *points = motion_points->data_float4() + attr_offset * num_points; - for (size_t i = 0; i < num_points; ++i) { - const PointCloud::Point point = pointcloud->get_point(i); - BoundBox bounds = BoundBox::empty; - point.bounds_grow(points, radius, bounds); + for (size_t i = 0; i < num_points; ++i) { + const PointCloud::Point point = pointcloud->get_point(i); + BoundBox bounds = BoundBox::empty; + point.bounds_grow(points[i], bounds); - const size_t index = step * num_points + i; - aabb_data[index].minX = bounds.min.x; - aabb_data[index].minY = bounds.min.y; - aabb_data[index].minZ = bounds.min.z; - aabb_data[index].maxX = bounds.max.x; - aabb_data[index].maxY = bounds.max.y; - aabb_data[index].maxZ = bounds.max.z; + const size_t index = step * num_points + i; + aabb_data[index].minX = bounds.min.x; + aabb_data[index].minY = bounds.min.y; + aabb_data[index].minZ = bounds.min.z; + aabb_data[index].maxX = bounds.max.x; + aabb_data[index].maxY = bounds.max.y; + aabb_data[index].maxZ = bounds.max.z; + } } } diff --git a/intern/cycles/scene/pointcloud.cpp b/intern/cycles/scene/pointcloud.cpp index f5b6a7f2050..bdd76cf530e 100644 --- a/intern/cycles/scene/pointcloud.cpp +++ b/intern/cycles/scene/pointcloud.cpp @@ -34,7 +34,7 @@ void PointCloud::Point::bounds_grow(const float4 &point, BoundBox &bounds) const float4 PointCloud::Point::motion_key(const float3 *points, const float *radius, - const float3 *point_steps, + const float4 *point_steps, size_t num_points, size_t num_steps, float time, @@ -56,7 +56,7 @@ float4 PointCloud::Point::motion_key(const float3 *points, float4 PointCloud::Point::point_for_step(const float3 *points, const float *radius, - const float3 *point_steps, + const float4 *point_steps, size_t num_points, size_t num_steps, size_t step, @@ -73,10 +73,7 @@ float4 PointCloud::Point::point_for_step(const float3 *points, step--; } const size_t offset = step * num_points; - return make_float4(point_steps[offset + p].x, - point_steps[offset + p].y, - point_steps[offset + p].z, - radius[offset + p]); + return point_steps[offset + p]; } } @@ -189,10 +186,10 @@ void PointCloud::compute_bounds() Attribute *attr = attributes.find(ATTR_STD_MOTION_VERTEX_POSITION); if (use_motion_blur && attr) { size_t steps_size = points.size() * (motion_steps - 1); - float3 *point_steps = attr->data_float3(); + float4 *point_steps = attr->data_float4(); for (size_t i = 0; i < steps_size; i++) - bnds.grow(point_steps[i]); + bnds.grow(float4_to_float3(point_steps[i]), point_steps[i].w); } if (!bnds.valid()) { @@ -204,10 +201,10 @@ void PointCloud::compute_bounds() if (use_motion_blur && attr) { size_t steps_size = points.size() * (motion_steps - 1); - float3 *point_steps = attr->data_float3(); + float4 *point_steps = attr->data_float4(); for (size_t i = 0; i < steps_size; i++) - bnds.grow_safe(point_steps[i]); + bnds.grow_safe(float4_to_float3(point_steps[i]), point_steps[i].w); } } } diff --git a/intern/cycles/scene/pointcloud.h b/intern/cycles/scene/pointcloud.h index 11676ffba90..c4a8d7f0e96 100644 --- a/intern/cycles/scene/pointcloud.h +++ b/intern/cycles/scene/pointcloud.h @@ -28,14 +28,14 @@ class PointCloud : public Geometry { float4 motion_key(const float3 *points, const float *radius, - const float3 *point_steps, + const float4 *point_steps, size_t num_points, size_t num_steps, float time, size_t p) const; float4 point_for_step(const float3 *points, const float *radius, - const float3 *point_steps, + const float4 *point_steps, size_t num_points, size_t num_steps, size_t step,