prochitecture / bpypolyskel

A port of Botffy/polyskel library for Blender that outputs polygons formed by a straight skeleton. 'pip install mathutils' to use it as a general purpose library independent of Blender.
GNU General Public License v3.0
52 stars 5 forks source link

Bugs in first version of bpypolyskel #5

Open polarkernel opened 3 years ago

polarkernel commented 3 years ago

I just have commited a first version of the bpypolyskel modules. The demo-module requires the mathutils library to be installed in your interpreter using 'pip install mathutils'. A demo running in Blender will follow later.

Please report and discuss eventual bugs or otpimizations here.

vvoovv commented 3 years ago

The value for PARALLEL of 1.0e-2 was too agressive for this example. If set to 1.0e-3, it works fine. I have made some minor changes to _cleanskeleton() in order to avoid a program crash in case of destroyed skeletons at this place. But this would not help.

I am getting StopIteration error for _berlin_karlshorst_roemerweg62.py.txt.

polarkernel commented 3 years ago

I've fixed the indentation problem in the line 460 of bpypolyskel.py.

I've made some changes to have a more concise code ...

Thanks.

polarkernel commented 3 years ago

I am getting StopIteration error for berlin_karlshorst_roemerweg_62.py.txt

After hours of trials I stopped the attempt to remove spikes by cleaning the skeleton. It is impossible to find out, which small angles have to be removed and which not, there is not enough information available about their topology.

Now I tried now a completely new approach. I reduced the function _cleanskeleton() to just remove real ghost edges, which are parallel up to floating point precision. This function got renamed to removeGhosts(). In polygonize() I added some code to remove spikes in the faces, which is more precisely defined. Currently, it works for spikes with an inner angle of up to (1-cos(alpha))=0.01, set by PARALLEL.

Give it a try.

polarkernel commented 3 years ago

In the entry about straight skeletons on Wikipedia, I found the following remark:

Petr Felkel and Štěpán Obdržálek designed an algorithm for simple polygons that is said to have an efficiency of O(nr + n log r).[7][8] However, it has been shown that their algorithm is incorrect.

Maybe this explains the issues we have.

vvoovv commented 3 years ago

Give it a try.

Example berlin_karlshorst_robert_siewert_strasse_52.py.txt.

The last face created by the library is [58, 47, 48, 55, 54, 28, 29]. It doesn't start from a vertex of the original polygon (58 > 53).

polarkernel commented 3 years ago

It doesn't start from a vertex of the original polygon

Fixed.

vvoovv commented 3 years ago

_berlin_karlshorst_zwieseler_strasse40.py.txt

The face [106, 107, 151, 150, 146, 148, 147, 149, 143, 143, 142, 145, 102, 103, 154] Index 143 occurs twice.

The face [83, 84, 126, 126, 85, 86] Index 126 occurs twice.

vvoovv commented 3 years ago

_berlin_karlshorst_rheinpfalzallee82.py.txt

The face [84, 85, 99, 99, 98, 60, 61, 101] Index 99 occurs twice

The face [59, 60, 98, 98, 85, 86] Index 98 occurs twice

polarkernel commented 3 years ago

Duplicates in faces in berlin_karlshorst_zwieseler_strasse_40.py.txt berlin_karlshorst_rheinpfalzallee_82.py.txt

Fixed.

My motivation drops and drops. For instance for _berlin_karlshorst_zwieseler_strasse40.py, I found faces like this:

bug

skeletonize() still delivers crossed skeleton edges, which leads for polygonize() to: Garbage in, garbage out. I'm afraid to end with something like:

        if building == "berlin_karlshorst_zwieseler_strasse_40":
            ...
        elif building == "berlin_karlshorst_rheinpfalzallee_82":
            ...
        elif ...

I go now back to skeletonize(), even if I need to rewrite it.

vvoovv commented 3 years ago

_berlin_karlshorst_rheinpfalzallee82.py.txt

The weird face. Also its middle point doesn't lie on the face beneath it. image


Also an abandoned point on the edge of the straight skeleton: image

vvoovv commented 3 years ago

For instance for _berlin_karlshorst_zwieseler_strasse40.py, I found faces like this:

bug

Is the same problem (the screenshot from _berlin_karlshorst_zwieseler_strasse40.py)?

image

polarkernel commented 3 years ago

I go now back to skeletonize(), even if I need to rewrite it.

I commited a new version of bpypolyskel.py. Many bug fixes, tweaks, tricks and changes, many of them have some influence on the other. I hope it's now really a step forward. Lets's see how far we go with it.

vvoovv commented 3 years ago

That's certainly a milestone! The existing examples work ok.

The only problem is gaps that occur for some complex footprints. Let's consider them later.

The new relative simple footprint berlin_karlshorst_roemerweg_114.py.txt never ends for me.

polarkernel commented 3 years ago

The new relative simple footprint berlin_karlshorst_roemerweg_114.py.txt never ends for me.

Fixed.

The algorithm in mergeNodeClusters() produced crossed skeleton edges as result of merging their sources to an inappropriate new source:

2

These crossed edges created a garbled face in polygonize(). There, the algorithm for spikes removal delivered the identical face as result, ending (?) in an endless loop.

I rewrote the algorithm in removeGhosts(), such crossings get now resolved.

polarkernel commented 3 years ago

The only problem is gaps that occur for some complex footprints

Have you an example, where such gaps occur?

vvoovv commented 3 years ago

Another building in Karlshorst, vertex duplication again: berlin_karlshorst_heiligenberger_strasse_30.py.txt

The face [43, 44, 73, 47, 48, 76, 51, 52, 85, 77, 74, 71, 70, 66, 65, 42, 43, 82, 38, 39, 64, 66, 82] The indices 43, 66, 82 occur twice.

vvoovv commented 3 years ago

Have you an example, where such gaps occur?

I'll post it tomorrow.

polarkernel commented 3 years ago

Another building in Karlshorst, vertex duplication again: berlin_karlshorst_heiligenberger_strasse_30.py.txt

This was really a fancy bug! All attempts to find the reason for the error failed for a long time and finally I thought, there is a bug in the sorting algorithm of Python. Let me explain it a bit in detail, maybe other users encounter similar problems and this may help them. I was able to localize the bug in the sorting algorithm used to create a counter-clockwise embedding in poly2FacesGraph(). There was one single node of the skeleton, where this sorting didn't give a correct result:

1

The red dot in the center is a node vertice of the skeleton, while the crosses are the sinks of this node. Starting point is an unordered list with all these sinks, for instance [3,4,1,2]. The goal of the sorting is to create an order where the angles of the sinks decrease, for instance [4,1,3,2].

Because this appeared as an efficient method, I used the cross product between the vectors from the node to the sinks as argument for the comparison. Using the cross product, it is for instance easy to determine, that the vector to 1 is on the right of the vector to 4, which means 'greater' for the sorting algorithm. Like this, 2 is 'greater' than 1 and 4 is 'greater' than 2. But then, 1 becomes 'greater' than 4, which was the 'smallest' vector! Because the order of angles is cyclic, this method can't work, although it was the first time since I introduced it, that it failed.

Needed are the angles of the vectors in a range of 0 ... 2π. One method to get them would be to use math.atan2(), but this is very inefficient. After some search in the internetm I found a beautiful solution as answer by MvG to the question "Fastest way to sort vectors by angle without actually computing that angle" on stackoverflow. He introduces an extremely efficient pseudoangle() function, that returns a monotonic number between [0 ... 4], ideal for angle comparison.

I adapted this function into poly2FacesGraph(), it works fine and fast. Finally, the bug is fixed and I regained my confidence in Python's sorting algorithm.

vvoovv commented 3 years ago

That's incredible! I've managed to import the whole test area in Karlshorst! Congratulations.

Now to the gaps...

vvoovv commented 3 years ago

A gap is created for this relative simple building footprint: berlin_karlshorst_ehrenfelsstrasse_14.py.txt

image

image

I think the three faces on the image above should share a common vertex. Then there would be no gap.

I have also more complex footprints with quite noticeable gaps.

polarkernel commented 3 years ago

A gap is created for this relative simple building footprint: berlin_karlshorst_ehrenfelsstrasse_14.py.txt

I solved the issue for this footprint (but did not yet commit). Could you please post the other examples, so that I can try to generalize the solution? Thanks.

vvoovv commented 3 years ago

berlin_karlshorst_andernacherstrasse_4.py.txt

The red circles mark the prominent gaps. The green circle marks edge crossing.

image

vvoovv commented 3 years ago

Even more prominent gap:

moscow_nikolskaya_street_4_eastern_part.py.txt

image

polarkernel commented 3 years ago

The red circles mark the prominent gaps. Even more prominent gap:

The gaps occured due to a bug in the removing of vertices between almost parallel adjacent edges. Fixed now.

The green circle marks edge crossing.

This is not an edge crossing, but some kind of cluster:

Figure_1-22

The distance between these vertices is 0.268, while the mergeRange is currently set to 0.15 (this is the last default argument of polygonize()). Therefore these vertices do not get merged. This is still an experimental value, as I don't know, how large it can be without affecting the architecture of a building.

vvoovv commented 3 years ago

No gaps! Thank you so much!

I was preparing to write a message how happy I was. But...

berlin_karlshorst_eginhardstrasse_7.py.txt

Face [49, 50, 89, 53, 54, 84, 57, 58, 82, 83, 83, 35, 36, 77, 39, 40, 71, 43, 44, 73, 45, 46, 78]. Duplicated index 83.

polarkernel commented 3 years ago

I was preparing to write a message how happy I was. But...

Thanks for your confidence, but I think the surprise bag of this algorithm will keep us busy for a while, the rate of new bugs is still too high. But one day ...

Duplicated index 83.

The reason for this bug was an old code segment that cleaned the skeleton this time too much. I forgot it there, but its task is now done by the face cleaning algorithms, so I removed it. All old example footprints seem to work with this fix.

However, there is another surprise that came up with this example, we have again an extra edge:

1

However, this time it is neither a ghost edge nor a spike. Ghost edges consist of two overlapping edges starting parallel from one source, ending in different sinks. But in this case, two different sources have the same sink and are antiparallel, like this:

                   source  ----------------> sink<-------------------- source

But this situation may also be legal and, additionally, I found no criteria, which edge would have to be removed. What I could do is to check, wether two faces are in the same plane and merge them. But I don't think this would be very efficient. Any idea?

vvoovv commented 3 years ago

All test examples work now. I'll try more test areas in the coming days.

                   source  ----------------> sink<-------------------- source

The simplest solution is to leave it as is. Which is not that bad.

vvoovv commented 3 years ago
                   source  ----------------> sink<-------------------- source

How is that case different from the ghost edge one? Couldn't the same approach (checking if the edges are parallel) be used for that case as well?

polarkernel commented 3 years ago

How is that case different from the ghost edge one

For a ghost edge, as we had them until now, two edges are leaving in parallel the same source node. They overlap a while until the shorter of them end in a sink, while the other continues to a sink more far away. This situations is easy to detect and to fix.

vvoovv commented 3 years ago

Good news: the library processed a quite complex footprint.

image

vvoovv commented 3 years ago

Not that good news. A face consisting of only one vertex:

potsdam_karl_marx_strasse_32.py.txt

The face with the index 4 has the single vertex with the index 18.

polarkernel commented 3 years ago

Good news: the library processed a quite complex footprint

Great!! A challenge for every builder!

Not that good news. A face consisting of only one vertex

The bug started with the first merge, I encountered until now, that changed an architectural detail. Although the small footprint edge produced two vertices in the beginning, they get merged later into one, because they are closer than mergeRange (see arrow):

1

The only way to avoid this is to reduce mergeRange . The merged node gets then isolated in several steps by the spikes removal part and remains finally as a triangle face:

2

The final bug was then produced by the spikes removal part. It should only remove spikes that go out of the face, but the dot product accepted also a spike into a face to be removed. This bug is fixed now.

vvoovv commented 3 years ago

This bug is fixed now.

Got an impossible polygon. All vertices of the polygon should lie in the same plane. But it's clearly not the case for the selected polygon.

image

vvoovv commented 3 years ago

potsdam_grosse_weinmeisterstrasse_49d.py.txt

Vertex duplication in the face [33, 34, 49, 50, 46, 47, 32, 33, 46, 50, 49, 45, 47, 46]. Vertices 33, 49, 50, 46, 47 occur twice.

vvoovv commented 3 years ago

potsdam_grosse_weinmeisterstrasse_49d.py.txt

This problem happens if the footprint is located some distance away from the zero point in Blender. If the footprint covers the zero point, there is no problem.

polarkernel commented 3 years ago

Got an impossible polygon.

I still don't have a solution for this bug. I tried two different attempts, both without success:

The short edges in berlin_karlshorst_zwieseler_strasse_40 do not have square angles with their neighbors. I could try to exclude them. However, I don't know if this consideration would hold for other very short edges. I am currently lacking new ideas.

polarkernel commented 3 years ago

This problem happens if the footprint is located some distance away from the zero point in Blender. If the footprint covers the zero point, there is no problem.

Very strange! polygonize() already shifts the footprint, so that its center of gravity is on the origin of the coordinate system. When I did this move already outside polygonize(), then nonetheless within polygonize() a shift of about dx = 20.35e-4 and dy = 13.56e-4 was required. This is for sure an issue due to floating point precision. I'll try to find a way to avoid that.

vvoovv commented 3 years ago

Thank you for the explanation!

vvoovv commented 3 years ago

Got an endless cycle while debugging my test environment on a smaller data set: saint_petersburg_nevski_17.py.txt

polarkernel commented 3 years ago

_potsdam_grosse_weinmeisterstrasse49d.py This problem happens if the footprint is located some distance away from the zero point in Blender. If the footprint covers the zero point, there is no problem.

The floating point precision is not the reason for this bug. I created a Vector class that works with double-precision floats instead of the single-precision floats used in mathutils.Vector. The result was almost identical, just the runtime was 10 times longer. I don't think that the single-precision floats are really a problem, merging nodes and removing spikes repairs all their issues.

However, I was able to locate an issue deep inside the code for skeletonization. It is already present in Botffy's original code (and maybe already in the original by by Felkel and Obdržálek). Near the end of the event processing, there appears a strange situation (see left image below):

12

The thick red, green and yellow areas are the active LAV's at this time. The red dot is a split event to be processed. But there is a difference to all previous situtations for split events: The event is outside of its LAV, which is the yellow one in this case. When it is processed, the resulting LAV is a totally degenerated polygon (see the green polygon in the right image).

In the newest commit I have introduced a check, that prevents such events from being processed. This leads to a correct result for this example, while all the old examples are still running correctly. Maybe it's an intermediate solution, as I couldn't find yet the reason, that produces such situations.

Got an endless cycle while debugging my test environment on a smaller data set: saint_petersburg_nevski_17.py.txt

This example produces the same issue, when the new check is not introduced:

34

However, when omitting the incorrect split event, the process runs out of valid events, before the last LAV gets completely processed (the red dot is an invalid event):

5

At least there is no more an endless cycle for this example, however a new issue to solve.

vvoovv commented 3 years ago

As usual there a good and a not that good news.

The good news. All 1100 hipped roofs from the data set containing _saint_petersburg_nevski17.py.txt work without any problem.

The not that good one. I wanted to execute my test environment against all 320 thousand hipped roofs in the world. Unfortunately already the 4th in the data set was the greenhouse in the Royal Botanical Gardens in London that produced the StopIteration exception: london_temperate_house.py.txt

polarkernel commented 3 years ago

The good news. All 1100 hipped roofs ...

That's really good news! Thank your for this motivator!

The not that good one.

286 vertices!! How shall I debug that? Please remove it for the moment from your test set. For sure there will be smaller footprints with the same bugs. Or I will train an AI for debugging, that will be faster ;-)

vvoovv commented 3 years ago

Ok, I will add a condition in my test environment to exclude that building.

polarkernel commented 3 years ago

At least there is no more an endless cycle for this example, however a new issue to solve.

Here are my good news: I found the reason for the event that was outside its LAV. The class _LAVertex takes its previous and next edge to compute whether it is a reflex edge or not. But for buildings, these (original edges) are often almost parallel, so that the result is often wrong. However, for split events, their _LAVertex for the splitted LAV ias always convex. I added now an argument to the constructor of _LAVertex, that forces _LAVertex to be convex for split events. I could remove the intermediate solution, that prevented bad events from being processsed, and it still works. This fix solves all issues in

potsdam_grosse_weinmeisterstrasse_49d.py saint_petersburg_nevski_17.py

but not for london_temperate_house.py. :-(

vvoovv commented 3 years ago

Here are my good news

Thank you very much! The roofs look very nice now:

saint_petersburg_nevski_17.py: image

potsdam_grosse_weinmeisterstrasse_49d.py: image

vvoovv commented 3 years ago

Got another endless cycle in Berlin while testing the world data set: berlin_ossietzkystrasse_24.py.txt

The footprint appears to be quite simple.

vvoovv commented 3 years ago

Not in list error in removeGhosts(..): prague_wenzigova_458.py.txt

polarkernel commented 3 years ago

Got another endless cycle ...

I know two places, where I could detect and handle endless cycles. I think they may be very inconvenient for large test runs. What measures would you propose to handle them?

vvoovv commented 3 years ago

I check in my test environment if there are repeated face indices and write the building id to a log file. I can also catch exceptions in my test environment and also write the building id to the log gile as well.

It's certainly a good idea to throw an exception in the polygonize(..) code if there is an endless cycle.

I'd expect the all those cases will be fixed for the production version.

polarkernel commented 3 years ago

Got another endless cycle ... Not in list error in removeGhosts(..)

_berlin_ossietzkystrasse24.py was a very good example! It has a spike of about 1 cm in its original contour at the long edge at east. This guided me directly to the bug.

Just by curiosity, I tried once to replace all dot- and cross-products for mathutils.Vector by a double precision version, while the other operations remained original. With this version (not committed), _prague_wenzigova458.py gives a good result. It seems that floating point precision does matter after all.