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

align extruded solids according to the PartBuilder #217

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

hiaselhans
Copy link
Contributor

this addresses the following example:

import build123d as bd
import jupyter_cadquery as jcq

with bd.BuildPart(bd.Plane.XZ) as bp:
    with bd.BuildSketch(bd.Plane.XZ):
        rect = bd.Rectangle(50,50)
        rect2 = bd.Rectangle(40,40)

    bd.extrude(amount=5)
    bd.extrude(rect2, amount=15)

bp

before:
image

after:
image

with the change, calls to extrude align the solids according to the workplanes in the context

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (ef35afa) 83.65% compared to head (c1b47d3) 83.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #217      +/-   ##
==========================================
+ Coverage   83.65%   83.66%   +0.01%     
==========================================
  Files          19       19              
  Lines        5464     5468       +4     
==========================================
+ Hits         4571     4575       +4     
  Misses        893      893              
Impacted Files Coverage Δ
src/build123d/operations_part.py 79.31% <100.00%> (+0.58%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hiaselhans hiaselhans force-pushed the extrude_alignment branch 2 times, most recently from a34bf3a to b980b24 Compare April 19, 2023 20:38
@gumyr
Copy link
Owner

gumyr commented Apr 20, 2023

In this case the repositioning makes sense but it quickly gets complex for the user to understand. Ironically, I recently removed the last of this repositioning logic from the operations. See the comment in the docs here: https://build123d.readthedocs.io/en/latest/operations.html

It’s important to note that objects created by operations are not affected by Locations, meaning their position is determined solely by the input objects used in the operation.

I guess I should expand this warning to workplanes as well.

Here are a couple ways to do the same:

import build123d as bd

with bd.BuildPart(bd.Plane.XZ) as bp:
    with bd.BuildSketch(bd.Plane.XZ):
        rect = bd.Rectangle(50, 50)
    bd.extrude(amount=5)
    with bd.BuildSketch(bd.Plane.XZ):
        rect2 = bd.Rectangle(40, 40)
    bd.extrude(amount=15)

algebra = bd.Plane.XZ * bd.extrude(bd.Rectangle(50, 50), 5) + bd.Plane.XZ * bd.extrude(
    bd.Rectangle(40, 40), 15
)

One extra line in the builder solution or do the whole thing in one line with algebra.

@hiaselhans
Copy link
Contributor Author

Personally, i find it much more confusing to have an operation behave so differently when it is called with the explicit argument.
One time, the context matters, the other time it doesn't

How does it get complex with repositioning? and what were the reasons to remove the repositioning logic?

@gumyr
Copy link
Owner

gumyr commented Apr 20, 2023

Here was the last Issue on this: #215

The rule that operations are only a function of their parameters and not workplane or location contexts seems like the least complex for users to understand. I think the problem behind this specific issue is that the rect and rect2 are actually on Plane.XY which is not what you're expecting - isn't that true? If so, this is a common misunderstanding of how BuildSketch works which I've tried to remedy with some documentation. Not a great solution I admit.

@jdegenstein
Copy link
Collaborator

I currently believe that using an object outside of the "scope" it is created in should not be promoted as a typical use pattern. By "using" I mean applying an operation from the object (such as extrude).

@hiaselhans
Copy link
Contributor Author

On the contrary, I am totally fine with 2D object being unaligned.
The confusing part is that it is hard to understand when context applies and when it doesn't.

What i would have expected:

  • 2D objects / operations are defined in 2-dimensional space and thus only have a 2D but no 3D location inherent.
  • 3D objects / operations are have a 3D location/alignment. Whenever a 3D object is created it is aligned to the current context

context would consist of:

  • current 3D locations / orientations (Workplanes)
  • current 2D locations (e.g. GridLocations)
  • current Builder (3D/2D)

In this sense, i would expect the following to be valid:

with bd.BuildPart(bd.Plane.XZ) as part:
    # 3d-objects are being captured and oriented in the XZ plane in this context
    circle = bd.Circle(2)  # creates a 2D primitive -> not captured
    bd.extrude(circle, amount=10)  # extrudes the 2D primitive to a 3d Primitive, which is captured and aligned

Lifting the limitations in Builder.validate_inputs would be a way to make #218 work.

I would even go further and say that contexts can be nested and there is no "inertial system", so that in nested context the inner orientation is relative to the outer one.
In the following:

with bd.BuildPart(bd.Plane.XZ) as outer_part:
    with bd.BuildPart(bd.Plane.XY) as inner_part:
        bd.Cone(5, 1, 10)

jcq.show(outer_part, inner_part)

The cone in outer_part would point in the Y direction, while in inner_part it points towards the Z direction.
To my understanding that would remove the necessity of mode=Private in partbuilders and would pave the way for "library components".

@gumyr
Copy link
Owner

gumyr commented Apr 21, 2023

Although these design decisions are valid, they aren't what I've decided to implement for build123d. Everyone has a different perspective of what makes sense to them; for example most users just expect to extrude along the Face normal even though that isn't really a very stable reference to decide on an extrusion direction (the normal can be flipped or even change across the Face if it's non-planar).

The current rule that operations only depend on their inputs and not the workplane/locations around them is my attempt to make a simple rule that applies everywhere and most users seem to find that it makes sense. It seems as though you'd be more comfortable with the algebra api where you have full control of everything which is great, there is no "best" api.

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.

3 participants