Entities#add_face fails when using edge array from Entities#add_edges as input

Developers Ruby API
1

I noticed that Entities#add_face fails (returns a nil object) when I use an edge array from Entities#add_edges as the input parameter.

This demonstrates the behavior:

#require 'sketchup.rb'

def demonstrate_bug
    entities = Sketchup.active_model.entities

    ### CASE 1: Face from Edges via Entities#add_line ###
    p1 = Geom::Point3d.new([1,1,0])
    p2 = Geom::Point3d.new([4,7,0])
    p3 = Geom::Point3d.new([7,1,0])

    e1 = entities.add_line(p1,p2)
    e2 = entities.add_line(p2,p3)
    e3 = entities.add_line(p3,p1)

    edges1 = [e1, e2, e3]
    f1 = entities.add_face(edges1)

    result1 = f1.is_a?(Sketchup::Face) ? 'PASS' : 'FAIL'
    vertex_equal_1 = edges1[0].start == edges1[-1].end
    vertex_colocated_1 = edges1[0].start.position == edges1[-1].end.position

    puts "Case 1: Face via edges from Entities#add_line: #{result1}"
    puts "\tStart/end vertex objects equal: #{vertex_equal_1}"
    puts "\tStart/end vertex positions equal: #{vertex_colocated_1}"

    
    ### CASE 2: Face from Edge array via Entities#add_edges ###
    p4 = Geom::Point3d.new([11,1,0])
    p5 = Geom::Point3d.new([14,7,0])
    p6 = Geom::Point3d.new([17,1,0])

    edges2 = entities.add_edges(p4, p5, p6, p4)
    f2 = entities.add_face(edges2)

    result2 = f2.is_a?(Sketchup::Face) ? 'PASS' : 'FAIL'
    vertex_equal_2 = edges2[0].start == edges2[-1].end
    vertex_colocated_2 = edges2[0].start.position == edges2[-1].end.position
    
    puts "Case 2: Face via edges from Entities#add_edges: #{result2}"
    puts "\tStart/end vertex objects equal: #{vertex_equal_2}"
    puts "\tStart/end vertex positions equal: #{vertex_colocated_2}"


    ### CASE 3: Face from Points via Entities#add_face(pts_array) ###
    p7 = Geom::Point3d.new([21,1,0])
    p8 = Geom::Point3d.new([24,7,0])
    p9 = Geom::Point3d.new([27,1,0])

    f3 = entities.add_face(p7, p8, p9)

    result3 = f3.is_a?(Sketchup::Face) ? 'PASS' : 'FAIL'
    
    puts "Case 3: Face via points, using Entities#add_face: #{result3}"
end

When I run it I get the following:

891×409 55.2 KB

Best guess is that one or both of the following are happening:

  1. The add_edges method is returning edges which create a loop spatially but not by reference to the same vertex object. (This is definitely the case–whether it’s somehow my fault isn’t clear to me.)
  2. The add_face method is only doing loop testing by object equality of vertices, not position equality.

If I’m wrong and this is expected behavior I’d be interested to know.

2

I always use edges.find_faces so don’t really know why yours is failing on 2…

edges2.each{|e| e.find_faces}

john

3

Went back and tried find_faces on each edge from case 2 in the console. It returned a 0 for each one, so I’m guessing the loop detection in find_faces is also comparing vertex objects instead of positions.

4

Side note, this is still true in SU 2017 (17.0.18898). Original report was SU 2016.

5

If your first loop makes a face, then later on it interferes with the second loop’s face. etc etc.

Best to make your new geometry somehow separated from everything else, e.g. inside its own group.entities…

6

I went back and wrote another example where there is one set of edges and one face inside a group. The face still fails.

#require 'sketchup.rb'

def demonstrate_bug
    entities = Sketchup.active_model.entities

    group = entities.add_group

    p1 = Geom::Point3d.new([1,1,0])
    p2 = Geom::Point3d.new([4,7,0])
    p3 = Geom::Point3d.new([7,1,0])

    edges = group.entities.add_edges(p1, p2, p3, p1)
    face = group.entities.add_face(edges)

    result = face.is_a?(Sketchup::Face) ? 'PASS' : 'FAIL'
    vertex_equal = edges[0].start == edges[-1].end
    vertex_colocated = edges[0].start.position == edges[-1].end.position

    puts "Face via edges from Entities#add_edges, in group: #{result}"
    puts "\tStart/end vertex objects equal: #{vertex_equal}"
    puts "\tStart/end vertex positions equal: #{vertex_colocated}"
end

917×410 41.1 KB

7

You could simply add the face using the three points and get the edges made at the same time ?

face = group.entities.add_face(p1, p2, p3)

8

Yes, Entities#add_face(p1, p2, [...], pn) works fine–I’m reporting a bug for the method signature Entities#add_face(edgearray).

Edited because I originally had add_face(pts_array) when I meant add_face(edgearray).

9

@nballenger

This topic may also be of interest.

10

Thanks–I took a look at that while trying to clarify this bug, and it definitely seems similar / related. I think at this point there’s not much more to do (unless there’s a more formal bug submission process I’m not aware of), since it seems to be a problem with the API internals.

11

Thanks for posting such a clear reproduction example. :thumbsup:

The issue is certainly with add_edges and it doesn’t merge the vertices. You end up with overlapping vertices which really isn’t what SketchUp should be doing. I had a search in our bug tracker and found the conversation (SU-19100). There was an internal desire to fix it, but at the same time a concern that it could break existing extensions. Since It’s possible to create the face from the points it was deemed that this was the recommended workaround.

I do however notice that the documentation for add_edges (Class: Sketchup::Entities — SketchUp Ruby API Documentation) doesn’t mention any of this, and I filed an issue to have that improved. (SU-35694)

12

If this variant of the method doesn’t work correctly and isn’t going to be fixed, wouldn’t it be better to remove it from the API instead of relying on a “don’t do that” warning in the documentation? After all, if it is broken then one can assume there are no existing extensions that rely on it, hence removing it won’t break anything!

13

It’s not that easy, because it’s not fully broken. It fails to merge vertices when the edge loop closes. But it do create a set of edges. Removing the method would most certainly break extensions. The issue in question is a detail of the method. It’s possible that fixing it won’t Maybe we would reopen that issue and look at it.

In general one have to be really careful to what you change in an API - it’s the golden rule that whatever you release you are stuck with. Now, off course you change bugs - but with careful consideration.

14

@tt_su - Thanks for looking into it. Changes are definitely tricky to phase in for a library based API, so updating the documentation works for me.

15

@tt_su

Surely just changing the method wouldn’t break anything.

add_edges(pts_or_array, merge_vertices = false)
16

Another workaround, which uses the same number of lines of code, it makes a face from the previous created ‘edges’

group.entities.add_face(edges.collect{|e|e.vertices}.flatten.uniq)

The ‘add_face’ method will accept an array of points OR vertices, and this method collects the edges’ vertices in a unique list.

1 Like
17

Adding an optional argument would work. I can reopen the issue with this proposal if you want?

18

To me it’s not a big deal because I always create faces using points. But for others it might be beneficial.