DRC should warn about full-polys

[Chad Parker]

A case has been made that full-polys should be depreciated, as there are a number of anomalous behaviors that can be generated by it.

Mailing list discussion below.

From Britton (20200403)
-----------------------

I recently hit a polygon bug that caused connectivity errors. I reported it
here previously. I tried pcb-rnd and it had same problem. Igor debugged it as
shown in the below message.

He makes a convincing case that fullypoly is just broken. The pcb manual
should at least mention this issue, and probably the feature should at least
draw some kind of warning (maybe like some other things where you get a DRC
error warning you about the potential for pcb itself to get confused).

Britton

---------- Forwarded message ---------
From: <email address hidden>
Date: Sun, Mar 15, 2020 at 9:07 PM
Subject: Re: [pcb-rnd] connectivity bug -> fullpoly again
To: <email address hidden>

Hi Britton,

On Sun, 15 Mar 2020, Britton Kerin wrote:

>Stitched polygons are probably the culprit as usual, but I have no
>idea how. Somehow adding one trace in one place (the big fat one
>above the 2x3 header near the middle of the board) causes connectivity
>to be missed elsewhere. I've attached screenshots showing
>connectivity ok and missed, and .pcb files (pcb format) also.
>
>pcb version 4.2.0 exhibits the same behavior.

Thanks, managed to narrow it down.

Attached is the minimal test case that causes the problem, which is
clearly caused by the legacy "full poly" setting. Do a {c f} on the thick
line segment on the top and/or the top side of the poly - there will be no
connection.

The reason is simple: a known limitation of the code which turns into a
rendering bug that is called the "fullpoly setting". When doing a
connectivity walk of the data, you can take it as a normal search
algorithm on a graph. Nodes of the graph are board objects, e.g. lines or
polygons, edges are whether they overlap geometrically in our 2.5d space
or not.

In this graph a node is either marked (as galvanically connected) or not.
There are no split nodes that are marked partly, e.g. through all edges
going out to a specific direction of the node but not through other edges.
The same, speaking in the geometry domain: a board object is either
connected or not, there's no object that can have multiple parts, some
parts connected, others not.

And that's where the full poly troll comes in: if you cut your polygon in
half, like you did on your board with the thick line, you create two
disconnected islands. Those should be handled as separate connectivity
nodes in the graph, while they are really the very same object. This
causes a lot of trouble.

Now we have two independent bugs about this:

bug#1: if you {c f} on either half of the cut polygon in my example
file, you sill see the other half is marked too. This is clearly wrong, as
there's no connection between the two parts. But we can't do that, because
the whole polygon is a single object, a single node in the above graph.
This is not something we want to fix, we rather want to get rid of the
fullpoly flag, because in its current form it's just a bad idea (see
below at section V).

bug#2: there's always a 'first island', which is the only island for the
non-fullpoly polygon (which is the largest area island in that case). In
the fullpoly case, there may be further islands. Most of the code, like
the connectivity code will unfortunately look only for the first islandd
(which can be considered as a bug we want to fix).

Ok, the bright side now:

I. Quick workaround on the sepcific case

1. Go over the poly at 90mm;49mm

2. Right click, edit flags

3. untick the full polygon flag

4. draw a new polygon on the now removed upper section

(Of course with this you will have the same problem again when you cut
another polygon of yours in half - since all your polygons seem to have
the full poly flag)

II. Quick workaround, generic, CLI

1. execute this action to select all affected polygons:

query(select, @.p.flag.fullpoly == 1)

2. remove the fullpoly flag from all selected objects at once using the
following action:

propset(selection, p/flags/fullpoly, 0)

3. draw new polygons to patch the gaps

III. Quick workaround, generic, GUI

1. use the advanced search from the menu or pressing {s s}

2. click on the 'E' button of the first expression (labelled <edit me>)

3. in the tree, open the flag subtree on the bottom, select fullpoly, then
'==' the middle and '1' on the right

4. click ok, the expr edit dialog closes and you get back the advanced
search dialog with the expresison of II/1 filled in and action combo box
set to 'select'. Click on 'apply' and it will execute the expression.

5. execute the propedit action; this will bring up an aggregated list of
properties of all selected objects

6. navigate to p/flags/fullpoly in the tree on the left, this will show
the current value on the right (checkbox ticked in)

7. untick the checkbox on the right and click 'apply' -> this will unify
the fullpoly flag on all currently selected object to false. You should
see the result immediately both in the propedit dialog and on your board.

8. just like II/3.: draw missing polys to patch the gaps.

(8 steps... Sorry, the CLI is always more efficient than the GUI, hehe)

IV. long term solution, user side

Do not ever use the fullpoly flag. That simple. That feature is and always
has been just broken b design. A typical feature that leaked in over the
decades and looked like a good idea first but caused a lot of trouble
later. The guy who implemented it probably didn't really think over all
consequences in all parts of the code, but focused on how it was possible
in the poly code. So the result is that the poly code and rendering
handles it properly but pretty much everything else breaks on it.

Plus if any user wants to write an user script dealing with board data,
there's a 99% chance he will forget about multi-island full-polys. So it's
hardwired in the system that we will just have more and more of bugs like
these.

Your top layer is already a patchwork of polygons. You have 40 polygons
just on that onbe layer already. A few extra polygons after removing
fullpoly won't really make a diff.

V. long temr solution, developer side

The fullpoly flag is a dangerous thing that should not be used by users
and should be removed from the code. In fact I should just remove it
imediately - except that I can't because it would break compatibility with
the obsolete pcb format....

So the plan is this:

1. I will code an extended object, probably called pour, that will take
over the role of the fullpoly flag. It will be somewhat similar to the
full poly thing, except that it will really create and maintain separate
polygon objects for the islands, removing a huge complication from the
code (by allowing all parts of the code to assume one object is one
object). With this, you will get about the same functionality as with full
poly today, except it won't suffer from bug#1 or bug#2

2. I also have plans about a minor data model upgrade to support
polygon-side clearance values; this would help us a lot in compatibility
to EDA tools that really matter on the market (eagle and kicad) as it
would help us reproduce their global or "zone" based clearance mechanism.
I already know how to do this cheap and without breaking any compatibility
with our current data model.

3. In the same time I will mark the fullpoly flag deprecated. This means
whenever pcb-rnd meets it, a warning will be thrown and a link to 'how to
switch to the pour extended object' will be included.

4. independently of the above effort, I will look at whether I canfix
bug#2 for cheap. If yes, I will probably do that. But since I absolutely
do not want to fix bug#1 because of the simpler plans on the pour
extended object, the outcome of bug#2 decision will not affect the above
3.

(I am going to add this plan to the feature deprecation page)

Best regards,

Igor2

From DJ (20200403)
------------------
IIRC Peter and I talked about this long ago, and one solution we came up
with was to have a "polygon stack" so you could do things like "invert
this section" or "cut this polygon with this line" (different than
clearing a polygon with a trace). The user would edit the things at the
bottom of the stack (the original polygons, cut lines, etc), but the
system would see the "virtual polygons" at the top (result) of the
stack. It added a bunch of functionality and fixed the full poly
problem, but neither of us ended up implementing it.

So yeah, old problem. Bug? Feature? You decide :-)

[Chad Parker (parker-charles)]

Britton has prepared two patches that implement DRC warnings when fullpolys are detected. This patch implements the DRC warning.

[Chad Parker (parker-charles)]

This patch adds a test case.

[Chad Parker (parker-charles)]

Hi Britton-

These patches have been applied in a topic branch, LP1881485 of the geda-project pcb repository. The test file was moved to "tests/inputs/drctest-fullpoly_warning.pcb".

Can you explain the contents of the test file? Why the thru-holes, and why three different polys with fullpoly set?

We should also add polygons that do not have the flag set, to demonstrate that they are not flagged.

In preparation for some future work, I'd like to move this check to a separate function, and call that from the main DRC function instead of embedding it directly into the main function. Eventually, there will be a list of DRC items that can be enabled or disabled by preferences.

Thanks for the patches, sorry for the slow progress.
--Chad

[Britton Kerin (britton-kerin)]

On Sun, May 31, 2020 at 12:50 PM Chad Parker <email address hidden> wrote:
>
> Hi Britton-
>
> These patches have been applied in a topic branch, LP1881485 of the
> geda-project pcb repository. The test file was moved to "tests/inputs
> /drctest-fullpoly_warning.pcb".
>
> Can you explain the contents of the test file? Why the thru-holes, and
> why three different polys with fullpoly set?

IIRC I was trying to determine if the mini-display that shows up in DRC
scaled in some useful way. Same with the vias. I concluded that it
doesn't since unnaturally huge vias also don't end up scaled into the
DRC finder image thingy.

> We should also add polygons that do not have the flag set, to
> demonstrate that they are not flagged.

True. I'm pretty sure I checked this on my real-life file but could be
added to the test file. The test file wasn't really intended for
commit of course.

> In preparation for some future work, I'd like to move this check to a
> separate function, and call that from the main DRC function instead of
> embedding it directly into the main function. Eventually, there will be
> a list of DRC items that can be enabled or disabled by preferences.

Sounds good.

Britton

[Chad Parker (parker-charles)]

Okay, I think that branch LP1881485 is ready for merging.

I believe that you're correct that the DRC renderer doesn't consider the size of the violation when it generates the pixmap. One more thing to add to the to-do list...