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

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

Open
XCYRKDSDA opened this issue Oct 2, 2024 · 9 comments
Labels
wontfix This will not be worked on

Comments

@XCYRKDSDA
Copy link

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?

@XCYRKDSDA XCYRKDSDA changed the title The BuildSketch in the called method failed to add faces to the outside BuildPart context The BuildSketch in a method failed to add faces to the outside BuildPart context Oct 2, 2024
@gumyr gumyr added the wontfix This will not be worked on label Oct 2, 2024
@gumyr gumyr added this to the Not Gating Release 1.0.0 milestone Oct 2, 2024
@gumyr
Copy link
Owner

gumyr commented Oct 2, 2024

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
Copy link
Author

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
Copy link
Contributor

snoyer commented Oct 4, 2024

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
Copy link
Owner

gumyr commented Oct 4, 2024

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
Copy link
Contributor

snoyer commented Oct 4, 2024

@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
Copy link
Owner

gumyr commented Oct 4, 2024

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
Copy link
Owner

gumyr commented Oct 4, 2024

@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
Copy link
Contributor

snoyer commented Oct 4, 2024

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
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants