Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geometry.from_dxf can only ever import one shape #246

Open
mfeif opened this issue Sep 19, 2022 · 16 comments
Open

Geometry.from_dxf can only ever import one shape #246

mfeif opened this issue Sep 19, 2022 · 16 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mfeif
Copy link

mfeif commented Sep 19, 2022

Describe the bug
When importing shapes from a clean/well-formed DXF file, only one shape is ever found.

To Reproduce
Steps to reproduce the behaviour:

  1. Import shapes with Geometry.from_dxf()
  2. Notice that only one shape exists

Expected behaviour
When working with a clean/well-formed DXF file, .from_dxf() should be able to import multiple shapes for analysis.

Additional context
I have a clean DXF file with some closed polylines in it, and all other entities purged/removed. I can import this DXF into shapely. I can also import this file into the cad_to_shapely shim. In both cases, the multiple-shape geometry is preserved.

However, when I do the same via Geometry.from_dxf() only one shape ever comes back. Digging deeper into the code in from_dxf I found this:

my_dxf = c2s.dxf.DxfImporter(dxf_filepath)
my_dxf.process()
my_dxf.cleanup()

polygons = my_dxf.polygons
new_polygons = c2s.utils.find_holes(polygons)
if isinstance(new_polygons, MultiPolygon):
    return CompoundGeometry(new_polygons)
elif isinstance(new_polygons, Polygon):
    return Geometry(new_polygons)
else:
    print(f"No shapely.Polygon objects found in file: {dxf_filepath}")

Looking into find_holes, we find that this behavior is by design:

def find_holes(polygons : List[sg.Polygon]) -> sg.Polygon:
    """
    Construct single polygon from imported CAD goemetry. 
    Assumes there is only one parent shape (the one with the largest gross area.)
    Access external perimeter with *polygon.exterior*
    Access holes perimeter(s, if there are any) with *polygon.interiors*
    Returns:
        Shapely Polygon with holes 
    """
    for p in polygons:
        if p.interiors:
            return p

    #sort by areas, largest first.
    polygons.sort(key=lambda x: x.area, reverse=True)
    parent = polygons.pop(0)

    keepers = []
    for p in polygons:
        if p.within(parent):
            valid = True
            for q in polygons:
                if (p.intersects(q) or p.within(q)) and p is not q:
                    valid = False
           
            if valid: keepers.append(p)


    new = sg.Polygon(parent,holes=keepers)
    return new

So by using this utility function, sectionproperties can never support more than one shape in a DXF. This is not a "bug" in cad_to_shapely, per-se, as its the desired behavior, but by using this utility function, we're throwing out data.

I went through the analogous process by hand, NOT throwing out polygons and manually made a CompoundGeometry object, and it worked:

from cad_to_shapely.dxf import DxfImporter
from shapely.geometry.multipolygon import MultiPolygon

doc = DxfImporter('polylines.dxf')
doc.process()
doc.cleanup()
polys = MultiPolygon(doc.polygons)

g = CompoundGeometry(polys)
g.create_mesh(mesh_sizes=[0.1])

(this showed a little thumbnail of an object in a Jupyter Notebook)

@robbievanleeuwen
Copy link
Owner

Hi @mfeif thanks for raising this. Are you able to provide the .dxf file so I can reproduce? You may need to rename the file extension to something else e.g. .txt for it to be able to upload here.

@mfeif
Copy link
Author

mfeif commented Sep 20, 2022

Sure thing. Here's a suitable file. This is what it looks like:

dxf

Only the largest (by area) shape is imported via the .from_dxf() constructor, and it returns an instance of Geometry rather than CompoundGeometry.

bug.dxf.txt

@mfeif
Copy link
Author

mfeif commented Sep 20, 2022

I had a thought; if we go around the .find_holes() function, as I did in my manual example, we will lose the actual hole finding 😄 which may not be the best.

Seems like the detection of intersection and within there could be done while still preserving multiple polygons, though. 🤷

Maybe it's a feature request on that library?

@robbievanleeuwen
Copy link
Owner

Hi @mfeif, I see that your geometry is not connected. sectionproperties isn't really designed around unconnected geometries as the warping analysis is invalid, see warning here. However you can still perform a geometric analysis and plastic analysis on unconnected geometries.

If you have multiple connected regions, e.g. two rectangles directly adjacent to each other, my understanding is that this should work with CompoundGeometry.from_dxf().

I suspect cad-to-shapely was written with all this in mind. If you want this feature supported it may be worth, as you say, putting in a feature request there?

@mfeif
Copy link
Author

mfeif commented Sep 21, 2022

My mistake; that is a made-up DXF file that I made just to exhibit the behavior; it's not realistic.

The actual use case is in the retaining wall industry; the linkages look like this sort of thing:
pzc-b_generic_domestic_single

The shapes interlock, but may have a mm or few space between them in actuality. And sometimes they are "connected" as you say (as in the little shapes on the corners of the beam flanges). I made a closer simplified representation that looks like this:

ex

Running it through CompoundGeometry.from_dxf() results in only the beam shape being returned, so no, it doesn't work as you were expecting and I was hoping. I've attached the more appropriate dxf for your experimentation. Cad-to-shapely explicitly only ever returns one polygon, as per the code comments over there. This isn't a bug in section-properties, per-se, but it is not currently working as you (we) expect (returning multiple contiguous shapes). I could ask for a new function over there, as you suggest, but section-properties would still have to change how .from_dxf() works and call that function. Right?

(And I am looking for geometric analysis (finding the centroid and so on) not warping; so that's cool by me.)

ex.dxf.txt

@robbievanleeuwen robbievanleeuwen added the bug Something isn't working label Sep 22, 2022
@robbievanleeuwen
Copy link
Owner

Great, thanks for investigating this one @mfeif! @aegis1980 wondering if this is something that could be supported in cad-to-shapely?

@normanrichardson
Copy link
Contributor

@mfeif and @robbievanleeuwen From what I can tell this arose from the need to batch analyzing Geometry sections. Is this correct? This is a good discussion, and I'm happy to help with this. I'm going to offer a challenge to the described expected behavior of Geometry.from_dxf. I do think that this could rather be a utility feature/function.

The definition of Geometry is

"""Class for defining the geometry of a contiguous section of a single material.
. In my opinion, the from_dxf method is a classmethod of Geometry (the decorator should be there, but it is missing), and in that respect, it is behaving as expected. Data in the dxf is interpreted according to the definition of Geometry. For multiple closed polygons in a dxf file, the 1st is selected (I'm actually not sure of this logic). There may be a more elegant way to make this selection, but only one Geometry should be produced.

This perspective may be too rigid and maybe it is appropriate for a classmethod to return an array of objects, but I thought I would offer it up. Let me know your thoughts.

@mfeif
Copy link
Author

mfeif commented Sep 22, 2022

Fair point, but I am using CompoundGeometry.from_dxf() 😊

@aegis1980
Copy link

aegis1980 commented Sep 22, 2022

When I wrote cad-to-shapely, I put 'finding lots of shapes with holes' in the too hard box. Info is not implicit in 'dumb' 2d geometry so find_holes really a utility function.
Finding the holes when there is a single main closed section is easy, since holes have to be smaller (in area) than the parent, but the downside is the issue raised here.
Importing hatches, regions and/or blocks (in cad to shapely, @mfeif Thanks for that) might be a way to go - not excited about implementing myself! feel free to do so!!
Or recursively calling find_holes maybe, for those too lazy to so hatches/regions/blocks.

@robbievanleeuwen robbievanleeuwen added help wanted Extra attention is needed good first issue Good for newcomers labels Sep 23, 2022
@robbievanleeuwen
Copy link
Owner

robbievanleeuwen commented Sep 23, 2022

@normanrichardson with you on the classmethod aspect and agree that Geometry.from_dxf() should return a Geometry object with a single region, while CompoundGeometry.from_dxf() should return a CompoundGeometry object with multiple regions (not necessarily contiguous). Unfortunately, this is not currently the behaviour.

A current workaround for users, although tedious, would be to export each region as a separate dxf files. Import these as multiple Geometry objects and then use the + operator to combine these geometries into a CompoundGeometry object.

In terms of implementing the desired CompoundGeometry.from_dxf() behaviour, I don't have any experience with any of the dxf libraries and unfortunately don't have the time to learn it at the moment. I have put the help wanted and good first issue labels on this issue and would welcome anyone interested in tackling this, noting that it would probably be a joint PR for both cad-to-shapely and sectionproperties.

@mfeif
Copy link
Author

mfeif commented Sep 23, 2022

Importing hatches, regions and/or blocks (in cad to shapely, @mfeif Thanks for that) might be a way to go - not excited about implementing myself! feel free to do so!!
Or recursively calling find_holes maybe, for those too lazy to so hatches/regions/blocks.

I'm not following about the hatches and so on.

I think using the logic in .find_holes iteratively, preserving multiple geometries (except those that are holes) makes closer sense. I might even be able to tackle that since you've already done the hard part of figuring out how to do it.

@connorferster
Copy link
Collaborator

connorferster commented Oct 21, 2022

Hey folks,

We encountered similar problems when trying to write a test to discover if a CompoundGeometry is disjoint. As @aegis1980 pointed out, it's hard!

After several days of trying things out, the only solution I could come up with was to create a network of shapely Polygons based on their connectivity to each other. Similarily, I think we had to solve a similar problem when dealing with overlapping geometries with a hole going through both geometries.

I think we could use cad-to-shapely to import all geometries and perhaps by-pass the hole detection part. Does the hole detection assume that if a geometry is completely contained within another geometry it is a hole? To make this whole thing more complicated, is there not also the possibility of nested materials within each other which may or may not have a hole in them?

I think that @mfeif is correct that it would be good if .from_dxf() could work with multiple geometries. I think perhaps the best approach is to bypass the hole detection in cad-to-shapely and bring all geometries into a single CompoundGeometry. From there the user can assign materials and do additive/subtractive modelling of the geometries as required to achieve their desired result.

@robbievanleeuwen
Copy link
Owner

I agree this seems like the best approach.

Do you see the best way forward as this?

  • An option in cad-to-shapely to avoid hole detection, e.g. an optional parameter hole_detection: bool = True that is True by default but can be set to False if required
  • Modified code in sectionproperties to implement the above

@connorferster are you able to make a PR to cad-to-shapely to implement?

@aegis1980
Copy link

I added in dev branch of cad_to_shapely utils.py a function filter_polygons that'll return a list of inferred "topographical surfaces" from closed curves (shapely polygons with/without holes/both). This ought to address various quandaries bought up in this thread & others. Aim is to depreciate find_holes.

Got some bits and bobs to sort out, then will try and tie in with section-properties from_dxf/load_dxf and make pull request.

@robbievanleeuwen At moment section-properties load_dxf calls c2s.utils.find_holes. This island-finding stuff is kind of in hinterland "upstream" of section-properties and "downstream" of cad-to-shapely (why it in c2s.utils module)

Also some complications arise when you consider if user wants to import multiple sections from a single dxf (list of Geometry objects; I recall someone wanting to do that), or a single compound section (CompoundGeometry). I am going to aim at the latter in pull request on the grounds that if you want to import and analyse multiple sections user can sort it out 'upstream' in CAD package (multiple DXF files), or programmatically-by-yourself (somehow!)

@robbievanleeuwen
Copy link
Owner

Love your work @aegis1980!

@connorferster
Copy link
Collaborator

connorferster commented Nov 1, 2022 via email

@robbievanleeuwen robbievanleeuwen mentioned this issue Jun 12, 2023
@robbievanleeuwen robbievanleeuwen added enhancement New feature or request and removed bug Something isn't working labels Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants