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

Add Builder context-aware selectors #371

Merged
merged 7 commits into from
Nov 22, 2023

Conversation

MatthiasJ1
Copy link
Contributor

Allows for querying parts inside of a build context without needing to name the context.

Before

with BuildPart() as part:
    with BuildSketch() as sketch_1:
        Rectangle(5,5)
        fillet(sketch_1.vertices(), 1)
    extrude(amount=1)
    chamfer(part.edges() | Plane.XY, 0.2)
show_all()

After

with BuildPart() as part:
    with BuildSketch():
        Rectangle(5,5)
        fillet(vertices(), 1)
    extrude(amount=1)
    chamfer(edges() | Plane.XY, 0.2)
show_all()

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6c7956a) 95.82% compared to head (16180a1) 95.84%.
Report is 4 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #371      +/-   ##
==========================================
+ Coverage   95.82%   95.84%   +0.02%     
==========================================
  Files          24       24              
  Lines        7226     7246      +20     
==========================================
+ Hits         6924     6945      +21     
+ Misses        302      301       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MatthiasJ1
Copy link
Contributor Author

Tests failing due to #372

@gumyr
Copy link
Owner

gumyr commented Nov 9, 2023

This is an interesting idea but I'm not convinced this should be done. One often will use selectors on multiple objects - both the builder objects and user assigned instance variables - within the same design. Consider this example:

from build123d import *
from ocp_vscode import *

with BuildPart() as bp:
    Box(1, 1, 1)
    top = bp.faces().sort_by(Axis.Z)[-1]
    with BuildSketch(top) as bs:
        Rectangle(0.5, 0.5)
        chamfer(bs.vertices(), 0.05)
    extrude(amount=-0.5, mode=Mode.SUBTRACT)
    fillet(top.edges().filter_by(Axis.X), 0.1)
show(bp)

image

Being able to just use edges or vertices saves a few characters but introduces a whole new way to use selectors. I'm worried this will cause more confusion.

@MatthiasJ1
Copy link
Contributor Author

IMO this follows the same rules as other operations which implicitly refer to the current context unless otherwise specified. In your example every operation uses the default context values, except the final fillet where you are manually specifying a non-default target. Otherwise, for example, the first line of BuildPart would instead be bp.Box(1,1,1).

@MatthiasJ1
Copy link
Contributor Author

Just to clarify: this would still work with your example:

from build123d import *
from ocp_vscode import *

with BuildPart() as bp:
    Box(1, 1, 1)
    top = faces().sort_by(Axis.Z)[-1]
    with BuildSketch(top):
        Rectangle(0.5, 0.5)
        chamfer(vertices(), 0.05)
    extrude(amount=-0.5, mode=Mode.SUBTRACT)
    fillet(top.edges().filter_by(Axis.X), 0.1)
show(bp)

Copy link
Owner

@gumyr gumyr left a comment

Choose a reason for hiding this comment

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

Based on the feedback from Discord, let's go ahead with this change - thanks for opening the PR. There are some things that need to change though, see below.

These operations need to be added to the docs in the Cheat Sheet and the Operations section.

"""
Return Solids for the current builder context

Return either all or the solids created during the last operation.
Copy link
Owner

Choose a reason for hiding this comment

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

The Select enum actually has three values: ALL, LAST, and NEW. There may be more options in the future so it's probably best to reword this line of the description to make it more generic.

"faces",
"wires",
"edges",
"vertices",
Copy link
Owner

Choose a reason for hiding this comment

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

To make this complete, there should be solid, face, wire, edge and vertex versions as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be ok then? Much less to maintain since its just wrapping the existing functions.

def __gen_context_component_getter(f: Callable):
    @functools.wraps(f)
    def getter(select: Select = Select.ALL):
        context = Builder._get_context(f.__name__)
        if not context: raise RuntimeError(f"{f.__name__}() requires a Builder context to be in scope")
        return f(context, select)
    return getter

vertices = __gen_context_component_getter(Builder.vertices)
edges = __gen_context_component_getter(Builder.edges)
wires = __gen_context_component_getter(Builder.wires)
faces = __gen_context_component_getter(Builder.faces)
solids = __gen_context_component_getter(Builder.solids)

vertex = __gen_context_component_getter(Builder.vertex)
edge = __gen_context_component_getter(Builder.edge)
wire = __gen_context_component_getter(Builder.wire)
face = __gen_context_component_getter(Builder.face)
solid = __gen_context_component_getter(Builder.solid)

Copy link
Owner

Choose a reason for hiding this comment

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

It's a pleasure working with skilled programmers like yourself, I would never have come up with this solution but it looks great. I was worried that the sphinx documentation system would have trouble with this but it's happy. Go ahead with the PR and don't worry about the docs, I'll add these new functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likewise!

Returns:
ShapeList[Solid]: Solids extracted
"""
return Builder._get_context("solids").solids(select)
Copy link
Owner

Choose a reason for hiding this comment

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

When used in Algebra, there is no Builder context so this fails:

>>> from build123d import *
>>> square = Rectangle(10,10)
>>> corners = vertices()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/roger-maitland/Documents/build123d/src/build123d/build_common.py", line 1200, in vertices
    return Builder._get_context("vertices").vertices(select)
AttributeError: 'NoneType' object has no attribute 'vertices'

The context should be checked for None and if there isn't one available a RunTime exception should be generated to tell the user that this operation only applies to Builder mode (i.e. an Builder context must be in scope).

@@ -1176,6 +1176,77 @@ def localize(cls, *points: VectorLike) -> Union[list[Vector], Vector]:
return result


def solids(select: Select = Select.ALL) -> ShapeList[Solid]:
"""
Return Solids for the current builder context
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Return Solids for the current builder context
Return Solids from the current builder context

@gumyr
Copy link
Owner

gumyr commented Nov 15, 2023

There is a problem. In Python 3.9, this error occurs: E ImportError: cannot import name 'ParamSpec' from 'typing' (/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/typing.py) and we need support for 3.9 (I asked folks a while ago and it's still being used).

@MatthiasJ1
Copy link
Contributor Author

So I can type hint this using Python 3.9 but the issue is the VSCode type hinting engine, Pylance, doesn't fully support the 3.9 syntax. This means the autocomplete won't be perfect although I can get it to mostly work. PyCharm's engine, on the other hand, actually has better support for the 3.9 syntax although it doesn't actually follow the spec, it just does its own custom black-box magic interpretation.

@gumyr
Copy link
Owner

gumyr commented Nov 17, 2023

This seems to solve the problem:

if sys.version_info < (3, 10):
    from typing_extensions import ParamSpec, Concatenate
else:
    from typing import ParamSpec, Concatenate

@MatthiasJ1 MatthiasJ1 requested a review from gumyr November 21, 2023 20:15
@gumyr gumyr merged commit b8d6aa1 into gumyr:dev Nov 22, 2023
10 checks passed
@gumyr
Copy link
Owner

gumyr commented Nov 22, 2023

Thank you. I've added a table to the docs with all these new functions here: https://build123d.readthedocs.io/en/latest/operations.html
image

MatthiasJ1 pushed a commit to MatthiasJ1/build123d that referenced this pull request Dec 22, 2023
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.

2 participants