[Arch] Possible bug in cleanShape()

A forum dedicated to the Draft, Arch and BIM workbenches development.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
chennes
Veteran
Posts: 3884
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

[Arch] Possible bug in cleanShape()

Post by chennes »

The LGTM static analyzer catches an unreachable conditional at lines 189 and 190 of ArchReference.py:

Code: Select all

181            # separate lone edges
182            shapes = []
183            for edge in shape.Edges:
184                found = False
185                for solid in shape.Solids:
186                    if found:
187                        break
188                    for soledge in solid.Edges:
189                        if found:
190                            break
191                        if edge.hashCode() == soledge.hashCode():
192                            found = True
193                            break
194            else:
195                shapes.append(edge)
The condition at 189 can never be true, because the code that sets found = True (on line 192) then immediately calls break, which will exit the innermost loop. So right off, those two lines (189 and 190) can be removed. But a closer look seems to point to another problem here, which is that, based on the presence of the else clause to the outermost loop, the loop at 183 clearly expects to sometimes be terminated by a break. But it's not: only the inner two loops ever get terminated prematurely. My guess is that this block of code is actually supposed to have an additional if found: break, something like this:

Code: Select all

            shapes = []
            for edge in shape.Edges:
                found = False
                for solid in shape.Solids:
                    if found:
                        break
                    for soledge in solid.Edges:
                        if edge.hashCode() == soledge.hashCode():
                            found = True
                            break
                if found: # Added so that if the edge was found by the above search, we stop looking
                    break
            else:
                # Now this only happens if the search never terminates (e.g. if the edge was never found)
                shapes.append(edge)
Alas, I don't use Arch, so I don't know what this code is, what it does, or how it could be tested. Anyone?
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
carlopav
Veteran
Posts: 2062
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [Arch] Possible bug in cleanShape()

Post by carlopav »

Looks like this part of the method is meant to "# separate lone edges" and add them back to the shape (read part compound them) once the solids are fused together.
Since plural is used, i guess there can be a lot of lone edges, so if we break that while loop as you propose just the first of them will be correctly added to the final compound... did I get it?

I would rather change it like this:

Code: Select all

            # separate lone edges
            shapes = []
            for edge in shape.Edges:
                found = False
                for solid in shape.Solids:
                    for soledge in solid.Edges:
                        if edge.hashCode() == soledge.hashCode():
                            # the edge belongs to this solid, so it's not a lone edge, mark as true and break the loop
                            found = True
                            break
                        # else another edge will be compared
                    if found:
                        # the edge belongs to a solid, so try next edge
                        break
                if not found:
                    # the edge does not belong to any of the shape.Solids, so it's a lone edge
                    # append it to the shapes and try next edge, until every edge is checked
                    shapes.append(edge)
This code seems to be quite resources intensive...
follow my experiments on BIM modelling for architecture design
Post Reply