Page MenuHome

Fix T95738: Wrong "Bottom" selection for cone without base
ClosedPublic

Authored by Leon Schittek (lone_noel) on Feb 13 2022, 11:16 PM.

Details

Summary

Fix an edge case, where the "Bottom" selection output would only
select one vertex when the Top Radius was set to '0' and the
Fill Type to 'None'.


When using the cone mesh primitive node to create a cone with a point
at the top and Fill Type 'None' the "Bottom" selection output would
only select one vertex instead of the entire bottom rim.

This patch fixes that by correctly slicing the selection when the
selection is on the Point domain rather than the Face domain.

I tested the patch on the file in the bug report and made sure no
other selections were broken with the attached .blend file.

masterpatch
Bug report file
Test file with all cone selections

Diff Detail

Repository
rB Blender
Branch
Fix-cone-primitive-selection-edge-case (branched from master)
Build Status
Buildable 20536
Build 20536: arc lint + arc unit

Event Timeline

Leon Schittek (lone_noel) requested review of this revision.Feb 13 2022, 11:16 PM
Leon Schittek (lone_noel) created this revision.
This revision is now accepted and ready to land.Feb 18 2022, 8:02 PM

Actually, I found the logic a bit obscure. Does this look correct to you as well? Thanks for the test file, that's useful.

diff --git a/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc b/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
index e0923344421..187da279c78 100644
--- a/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
+++ b/source/blender/nodes/geometry/nodes/node_geo_mesh_primitive_cone.cc
@@ -524,11 +524,11 @@ static void calculate_selection_outputs(Mesh *mesh,
     if (config.bottom_is_point) {
       selection[config.last_vert] = true;
     }
+    else if (face) {
+      selection.slice(config.bottom_faces_start, config.bottom_faces_len).fill(true);
+    }
     else {
-      selection
-          .slice(config.bottom_faces_start,
-                 face ? config.bottom_faces_len : config.circle_segments)
-          .fill(true);
+      selection.slice(config.last_ring_verts_start + 1, config.circle_segments).fill(true);
     }
     attribute.save();
   }

@Jacques Lucke (JacquesLucke) I agree that that is clearer. Looks good to me and worked well when I tried it.