gumyr / build123d

A python CAD programming library
Apache License 2.0
402 stars 75 forks source link

Ability to create a BuildPart context on an existing part #108

Open bgolinvaux opened 1 year ago

bgolinvaux commented 1 year ago

I think that the ability to create a part with more than one context, spread across the source code, could be handy.

In one of my designs, I first wanted to build a basic enclosure, then work on some accessories (buttons, wheels..)

At some point in the code I needed to work on the enclosure again to create holes, etc.

The only way I have found is this rather inelegant snippet:

with BuildPart() as oshell02:
    Add(oshell.part)
    ... rest of operations

Wouldn't it be a good idea to be able to "reactivate" a part builder on an existing part?

Something like :

with BuildPart(existing_part=my_part) as oshell02:
   ... 

(existing_part is perhaps too long to type, but you get the idea.... or maybe BuildPart.makeFromPart(my_part) could do.)

A common pattern could then be for these builders to always bear the same name (my_part_builder) so that we could append or insert new operations while leaving the end of the source code (that perhaps performs calls such as show_object(my_part_builder.part)) intact.

TIA!

gumyr commented 1 year ago

It looks like it might be possible - here is the result of some limited experimentation:

from build123d import *

with BuildLine() as initial:
    Line((0, 0), (1, 0))

with BuildLine(resume=initial) as initial:
    ThreePointArc((1, 0), (0, 1), (-1, 0))

show_object(initial.line)

image

I don't know of a way to determine if the user has used the same instance name for the second with block, therefore the resume=initial parameter.

What do you think?

bgolinvaux commented 1 year ago

This is really fine, and exactly what I envisioned. I do not think that the variable name is accessible at this stage: the BuildPart constructor is first called, then is assigned to initial. (Even if it were through some locals() inspection magic, I do not think it would be wise. Being explicit in that situation looks like a good idea to me)

Thanks for working on this suggestion so fast!

gumyr commented 1 year ago

It's just a quick prototype - I'm concerned that it could be a lot more complex as the Builder, Workplane, and Location contexts may have changed resulting in hard to find strangeness. All I did was:

1) Add resume as a BuildLine parameter:

    def __init__(
        self,
        workplane: Union[Face, Plane, Location] = Plane.XY,
        resume: Builder = None,
        mode: Mode = Mode.ADD,
    ):
        self.initial_plane = workplane
        self.mode = mode
        self.line: Compound = None
        super().__init__(workplane, resume=resume, mode=mode)

2) Update the common Builder class:

    def __init__(
        self,
        *workplanes: Union[Face, Plane, Location],
        resume: Builder = None,
        mode: Mode = Mode.ADD,
    ):
        if resume:
            print("Resuming...")
            setattr(self, resume._obj_name, resume._obj)
            self.mode = resume.mode
            self.workplanes = resume.workplanes
            self._reset_tok = None
            self.builder_parent = None
            self.last_vertices = resume.last_vertices
            self.last_edges = resume.last_edges
            self.workplanes_context = None
            self.active: bool = False  # is the builder context active
            self.exit_workplanes = None
        else:
            self.mode = mode
            self.workplanes = workplanes
            self._reset_tok = None
            self.builder_parent = None
            self.last_vertices = []
            self.last_edges = []
            self.workplanes_context = None
            self.active: bool = False  # is the builder context active
            self.exit_workplanes = None            

I have some higher priority stuff to complete - e.g. finishing off the exporters - so I'll leave this feature for now but I will get back to it.

bgolinvaux commented 1 year ago

Thanks.

Actually, I am not really aware of what the state of a builder is, as opposed to its contained part, so I cannot appreciate how dangerous this might be.

I notice that resume is the previous builder in your case. Wouldn't it be possible to use the part instead, and start a building context anew, exactly like the way I am doing it now as a workaround?

with BuildPart() as my_part_builder:
  # first building operations
  ...

my_part = my_part_builder.part

# other code
...

with BuildPart(part=my_part) as my_part_builder:
  # the builder would create a fresh BuildPart instance and would call 
  # Add(part) right after construction
  ...

I assume that the difference means that we cannot save the state that is contained in the builder rather than the part, but I think that it would perhaps be a good practice to not rely on the builder state and exiting the builder once everything has been used, except the part itself, perhaps.

Please accept my apologies if this is not feasible or even plain stupid: until I dive into b123d source code more, these are just very vague suggestions

TIA!

gumyr commented 1 year ago

These are great discussions. User feedback (of cq) is what b3d was created from so please keep it up. Even vague suggestions can be inspiration for improvement.

With respect to Add - setattr(self, resume._obj_name, resume._obj) basically does this for all of the builders (this code is in the builder's base class). Each builder must create _obj and _obj_name properties where _obj is assigned line, sketch or part in the appropriate builder and _obj_name is just the string version of this. Therefore, the part attribute is copied from the resume BuildPart instance to the self BuildPart instance.

There are a few other things that could to copied from the resume instance:

The parent would have to be re-established as it's likely gone but I would think users would expect that.

If these objects weren't copied from the resume instance then it's just an Add which is just a slightly optimized version of what you're already doing.

Maybe it isn't all that problematic...