gumyr / build123d

A python CAD programming library
Apache License 2.0
541 stars 90 forks source link

The BuildSketch in a method failed to add faces to the outside BuildPart context #723

Open XCYRKDSDA opened 1 month ago

XCYRKDSDA commented 1 month ago

I tried the following three scenarios:

  1. with BuildPart():
        with BuildSketch():
            Circle(10)
        extrude(amount=10)

    This worked, of course.

  2. def make_cylinder():
        extrude(amount=10)
    
    with BuildPart() as builder:
        with BuildSketch():
            Circle(10)
        make_cylinder()

    This also worked.

  3. def make_cylinder():
        with BuildSketch():
            Circle(10)
        extrude(amount=10)
    
    with BuildPart() as builder:
        make_cylinder()

    This failed at the extrude step with error "A face or sketch must be provided". I looked into it a little bit. After finishing the BuildSketch process, the pending_faces field of the BuildPart context was supposed to contain a face, but it didn't. Why?

gumyr commented 1 month ago

In Python, the with .. construct doesn't follow Python scoping rules which causes a great deal of confusion when using them in functions like this. build123d does some inspection magic to attempt to emulate appropriate scoping rules but it is easily foiled as illustrated here where the BuildSketch in the function is isolated from the BuildPart outside of the function.

If you're an expert in this area of Python I'd welcome any improvements that could be made. Otherwise, this isn't supported. Of course, there are other ways to achieve the same goal.

XCYRKDSDA commented 4 weeks ago

Interesting. I thought the builder would use some kind of global variables to maintain the context. So for now, how can I encapsulate common operations under builder mode into a function?

snoyer commented 4 weeks ago

In Python, the with .. construct doesn't follow Python scoping rules which causes a great deal of confusion when using them in functions like this.

Can you elaborate on that? Isn't Python's with statement just syntactic sugar for context managers, i.e. built-in boilerplate to call __enter__ and __exit__ with some exception handling thrown in?

gumyr commented 4 weeks ago

In Python, the with .. construct doesn't follow Python scoping rules which causes a great deal of confusion when using them in functions like this.

Can you elaborate on that? Isn't Python's with statement just syntactic sugar for context managers, i.e. built-in boilerplate to call __enter__ and __exit__ with some exception handling thrown in?

Maybe I'm not using the correct terminology here, but yes with blocks are implemented as context managers that use __enter__ and __exit__ methods. The problem is that until the __exit__ method is called that context is still active which gets to be very confusing when instantiating a class that also has context managers as they now have a parent. This can result in unexpected behavior as objects are passed back when one wouldn't normally expect them to.

snoyer commented 4 weeks ago

@gumyr I'm not sure I understand all of the implications for every possible build123d usecase, but for the issue at hand, looking higher up the stack of frames to decide the value of same_scope in Builder.__enter__ would make the third snippet work https://github.com/snoyer/build123d/blob/efe9d75904a75cb4ce3f027d06bd46af392922b4/src/build123d/build_common.py#L241-L250 let me know if that makes sense to you and if you want a PR

gumyr commented 4 weeks ago

Interesting. I thought the builder would use some kind of global variables to maintain the context. So for now, how can I encapsulate common operations under builder mode into a function?

Global variables aren't used as that could make multi-threading fail.

All build123d "operations" are just Python functions, take a look at operations_generic.py which has many examples (these generic operations operate with more than one builder type). You'll find _add_to_context as shown here:

        if context is not None:
            context._add_to_context(new_wire, mode=Mode.REPLACE)

which is instructing the builder to combine this new object(s) with its existing object with the given mode.

However, from the examples you've shown above it appears that you want to create custom objects which is described here: custom-objects By creating a sub-class of say BasePartObject you will have created a new part that is fully compatible with both Builder and Algebra mode. The example in the docs creates a playing card Club which can now be used as:

with BuildPart() as obj:
    with BuildSketch() as card:
        Club(height=10)
        Circle(2, mode=Mode.SUBTRACT)
    extrude(amount=1)
gumyr commented 4 weeks ago

@gumyr I'm not sure I understand all of the implications for every possible build123d usecase, but for the issue at hand, looking higher up the stack of frames to decide the value of same_scope in Builder.__enter__ would make the third snippet work https://github.com/snoyer/build123d/blob/efe9d75904a75cb4ce3f027d06bd46af392922b4/src/build123d/build_common.py#L241-L250 let me know if that makes sense to you and if you want a PR

This is how it works currently, see: https://github.com/gumyr/build123d/blob/dev/src/build123d/build_common.py#L234 However, improvements are welcome.

snoyer commented 4 weeks ago

This is how it works currently, see: dev/src/build123d/build_common.py#L234 However, improvements are welcome.

yes that is the function I proposed a modification of. Instead of checking that Builder._get_context()._python_frame is inspect.currentframe() you could check that it is somewhere in inspect.stack() basically, meaning that you could find the parent context further up in the call stack. That allows the failing example from the original message to work but I don't know enough about the general design and intent of your library to know if it is actually an improvement in all cases.

XCYRKDSDA commented 4 weeks ago

All build123d "operations" are just Python functions, take a look at operations_generic.py which has many examples (these generic operations operate with more than one builder type).

However, from the examples you've shown above it appears that you want to create custom objects which is described here: custom-objects By creating a sub-class of say BasePartObject you will have created a new part that is fully compatible with both Builder and Algebra mode.

This is really helpful. Thank you!