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

Fix boolean_or operations #2804

Merged
merged 3 commits into from
May 30, 2024
Merged

Fix boolean_or operations #2804

merged 3 commits into from
May 30, 2024

Conversation

fp-matt
Copy link
Contributor

@fp-matt fp-matt commented May 30, 2024

When trying to merge two component references using the or operator with gf.boolean(...), as the docs suggest, the output doesn't merge A and B unless they explicitly overlap. Even if the references share an edge, the output is not merged. Additionally, the output is currently dependent on the order in which the references are given (e.g. A=ref1, B=ref2 vs. A=ref2, B=ref1), and in one of the cases, the output appears mirrored (though it is actually just mistranslated, as shown later).

image

Issues:

  • A | B:

    • Output is effectively mirrored
    • Isn't merged even though the edges of the two polygons coincide
  • B | A:

    • Polygons were not properly translated relative to each other, though the output is a single merged polygon

Expected behavior
I would expect a boolean or to maintain the relative positions of the input polygons and also merge everything into a singular output polygon (in the case that the inputs share at least an edge).

The mistranslation issue was being caused by this bit of code here:

    for r1, r2 in zip(
        a.begin_shapes_rec(layer_index1),
        b.begin_shapes_rec(layer_index2),
    ):
        r1 = kf.kdb.Region(r1)
        r2 = kf.kdb.Region(r2)
        if isinstance(A, kf.Instance):
            r1.transform(A.cplx_trans)
        if isinstance(B, kf.Instance):
            r1.transform(B.cplx_trans)        # <---
        f = boolean_operations[operation]
        r = f(r1, r2)
        r = c.shapes(layer_index).insert(r)

    return c

This was applying both transformations from A and B to r1.

From what I can tell, however, the issue with the output not merging seems like it might be due to how the or operation is performed in the backend (maybe within KLayout itself?) rather than an issue with GDSFactory/KFactory, but I don't think I'm familiar enough with any of the backend stuff to really say with any degree of certainty.

Regardless, especially since the docs suggest using gf.boolean(...) to merge components/references, I would expect boolean_or to merge any regions that are touching.

The solution I've found is to use the interacting(...) function in KLayout's Region class to detect the cases where two non-overlapping polygons share an edge so that they can be merged.

I am not sure how this change effects performance, however, or if a more elegant solution exists, so any suggestions/edits are welcome.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've reviewed this pull request using the Sourcery rules engine. If you would also like our AI-powered code review then let us know.

@fp-matt
Copy link
Contributor Author

fp-matt commented May 30, 2024

Code used to generate the included screenshot:

import gdsfactory as gf

width = 500
y_offset = 100


@gf.cell
def before_bool_or(width: float = width,
                   length: float = 1500,
                   ) -> gf.Component:

    c = gf.Component("before_bool_or")

    tall_ref = c << gf.components.straight(length=length,
                                           width=width)

    short_ref = c << gf.components.straight(length=length / 2,
                                            width=width - y_offset)

    tall_ref.connect('o1',
                     short_ref.ports['o2'],
                     allow_width_mismatch=True,
                     )

    short_ref.dmovey(-y_offset / 2)

    c << gf.components.text(text="None",
                            size=100,
                            position=(c.dx, c.dy),
                            layer="TEXT",
                            justify="center",
                            )

    return c


@gf.cell
def after_bool_or_AB(width: float = width,
                     length: float = 1500,
                     ) -> gf.Component:
    c = gf.Component("after_bool_or_1")

    tall_ref = c << gf.components.straight(length=length,
                                           width=width
                                           )

    short_ref = c << gf.components.straight(length=length / 2,
                                            width=width - y_offset
                                            )

    tall_ref.connect('o1',
                     short_ref.ports['o2'],
                     allow_width_mismatch=True,
                     )

    short_ref.dmovey(-y_offset / 2)

    merged_c = gf.boolean(A=tall_ref,
                          B=short_ref,
                          operation="or",
                          layer=1,
                          )

    merged_c = merged_c.copy()

    merged_c << gf.components.text(text="A | B",
                                   size=100,
                                   position=(merged_c.dx, merged_c.dy),
                                   layer="TEXT",
                                   justify="center",
                                   )

    return merged_c


def after_bool_or_BA(width: float = width,
                     length: float = 1500,
                     ) -> gf.Component:
    c = gf.Component("after_bool_or_2")

    tall_ref = c << gf.components.straight(length=length,
                                           width=width
                                           )

    short_ref = c << gf.components.straight(length=length / 2,
                                            width=width - y_offset
                                            )

    tall_ref.connect('o1',
                     short_ref.ports['o2'],
                     allow_width_mismatch=True,
                     )

    short_ref.dmovey(-y_offset / 2)

    merged_c = gf.boolean(A=short_ref,
                          B=tall_ref,
                          operation="or",
                          layer=1,
                          )

    merged_c = merged_c.copy()

    merged_c << gf.components.text(text="B | A",
                                   size=100,
                                   position=(merged_c.dx, merged_c.dy),
                                   layer="TEXT",
                                   justify="center",
                                   )
    return merged_c


if __name__ == "__main__":
    from gdsfactory import pdk

    pdk.get_active_pdk().activate()

    c = gf.Component("bool_or_before_and_after_comparison")

    before = c << before_bool_or()
    after1 = c << after_bool_or_AB()
    after2 = c << after_bool_or_BA()

    before.dmovey(3 * width / 2)
    after2.dmovey(-3 * width / 2)

    c.show()

Co-authored-by: Sebastian Goeldi <44963764+sebastian-goeldi@users.noreply.github.com>
Copy link
Contributor

@joamatab joamatab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Matthew and Sebastian!

@joamatab joamatab merged commit 162d54a into gdsfactory:main May 30, 2024
4 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants