-
Notifications
You must be signed in to change notification settings - Fork 391
Tagging objects in the chain #252
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 94.91% 94.95% +0.03%
==========================================
Files 18 18
Lines 4229 4280 +51
==========================================
+ Hits 4014 4064 +50
- Misses 215 216 +1
Continue to review full report at Codecov.
|
|
Bernd had a problem on Google Groups that this PR would significantly help with. If he had of posted it as an issue on GitHub I would have maked it as solved by this PR, so I'm going to attach it to this comment instead. To summarise all the below, I just interrupt the chain to create the desired workplane, leave it in Bernd's code: import cadquery as cq
grundkreisradius=30
gesamttiefe=50
glasdicke=9
schraubenabstand=30
schraubenabstandhalb=schraubenabstand/2
schraubengangdurchmesser=5
grundscheibendickeunterschraube=3
schraubenkopfdicke=10
result = (cq
.Workplane("front")
.circle(grundkreisradius)
.workplane(offset=50)
.center(grundkreisradius/2, 0)
.rect(20, 20)
.loft(combine=True)
.faces(">Z")
.workplane()
.rect(glasdicke,200)
)
# Here, I want to do something like this:
# rodWorkplane = (result
# .cutBlind(-40)
# .faces("<Z")
# cut a cube out and save a certain face of that cut-out cube
result = (result
.cutBlind(-40)
.faces("<Z")
.workplane()
.center(schraubenabstand / 2, 0)
.hole(schraubenkopfdicke)
.faces("<Z")
.workplane()
.center(-schraubenabstand / 2, 0)
.hole(schraubenkopfdicke)
.faces("<Z")
.circle(grundkreisradius)
.extrude(grundscheibendickeunterschraube)
.faces("<Z")
.workplane()
.center(schraubenabstandhalb, 0)
.hole(schraubengangdurchmesser)
.faces("<Z")
.workplane()
.center(-schraubenabstandhalb,0)
.hole(schraubengangdurchmesser)
)
#now I could do:
# result=result.rodWorkplane.circle(4).extrude(100)
# instead of the above, this works, too, but it is difficult, clumsy and quirky:
endloch = (cq
.Workplane("right")
.workplane(offset=-60)
.center(-20, 0)
.circle(4)
.extrude(100)
)My code: import cadquery as cq
grundkreisradius=30
gesamttiefe=50
glasdicke=9
schraubenabstand=30
schraubenabstandhalb=schraubenabstand/2
schraubengangdurchmesser=5
grundscheibendickeunterschraube=3
schraubenkopfdicke=10
result = (cq
.Workplane("front")
.circle(grundkreisradius)
.workplane(offset=50)
.center(grundkreisradius/2, 0)
.rect(20, 20)
.loft(combine=True)
.faces(">Z")
.workplane()
.rect(glasdicke,200)
.cutBlind(-40)
)
# Here, I want to do something like this:
# rodWorkplane = (result
# .cutBlind(-40)
# .faces("<Z")
# cut a cube out and save a certain face of that cut-out cube
rod_wp_obj = (result
.faces(">Z[-2]")
.workplane()
.transformed(rotate=(0, -90, 0))
.center(10, 0)
)
result = (result
.faces("<Z")
.workplane()
.center(schraubenabstand / 2, 0)
.hole(schraubenkopfdicke)
.faces("<Z")
.workplane()
.center(-schraubenabstand / 2, 0)
.hole(schraubenkopfdicke)
.faces("<Z")
.circle(grundkreisradius)
.extrude(grundscheibendickeunterschraube)
.faces("<Z")
.workplane()
.center(schraubenabstandhalb, 0)
.hole(schraubengangdurchmesser)
.faces("<Z")
.workplane()
.center(-schraubenabstandhalb,0)
.hole(schraubengangdurchmesser)
)
#now I could do:
# result=result.rodWorkplane.circle(4).extrude(100)
# instead of the above, this works, too, but it is difficult, clumsy and quirky:
#endloch = (cq
# .Workplane("right",)
# .workplane(offset=-60)
# .center(-20, 0)
# .circle(4)
# .extrude(100)
# )
result = (result
.copyWorkplane(rod_wp_obj)
.workplane(offset=-25)
.circle(4)
.extrude(100)
)edit: I forgot to mention there is a small offset applied to the rod in my example. I have no idea where it is coming from and it's super annoying! |
|
@dcowden This is touching on design decisions and code commits that you made early on. Do you have any comments? |
|
@marcus7070 I think this feature will be heavily used. The need for something like this comes up in discussion and requests for help fairly often. With a little forethought on the part of the user, they should be able to approximate functionality similar to selection by operation. You can have a look at the related issues that are mentioned here. For the example(s), if you create stand-alone examples, I think it would be good to add them between Ex024 and Ex100 rather than the contrib repo so that users are exposed to it early on. |
|
@jmwright thanks for tagging me in.! @marcus7070 , i love it! This feature would be very helpful as-is, and I would definitely not have a problem merging. One comment on the interface, though-- It would be super cool if the tagging syntax contemplated tagging other types of objects, and if selectors could in turn select tagged objects. The original example would change very slightly to: This has a few benefits:
What do you guys think? |
|
@dcowden Will that have any weird consequences for the stack if other types of objects are selected? |
|
@jmwright it definitely makes the implementation more complex, since selectors can already return CQ objects representing 1 or more faces, edges, and points, it seems workable. Tagging faces, edges, and points would imply that its necessary to keep a copy of the original object around, so that they can be returned even if they have been modified or deleted by later operations. |
|
I'm out of time for the moment, but I'm working on what @dcowden suggested. import cadquery as cq
part = (cq
.Workplane("XY")
.polygon(3, 2)
.extrude(1)
.tag("triangle")
.sphere(2)
.faces(">X and >Y", tag="triangle")
.workplane()
.circle(1)
.extrude(2)
) |
|
@marcus7070 thanks for working on this, I love it! This functionality will be a huge benefit for more complex objects! |
|
Tags in selectors allows a much neater solution to Bernd's problem as well. Instead of breaking the chain and creating a result = (cq.Workplane("XY")
# lots of operations snipped
.cutBlind(-40) # cut the slot
.tag("slotted")
# lots of operations snipped
.faces(">Z[-2]", tag="slotted") # select the third furthest face in the z direction in the parent object tagged with "slotted"
.workplane()
.transformed(rotate=(0, -90, 0))
# position and create the rod
) |
I really like the idea of tagging (needed that before) and copying workplanes. Great addon to cadquery! Out of curiosity, I monkey patched your So it doesn't seem to be the code or the |
ce614e2 to
4fd5784
Compare
|
I'll try to model some projects using this branch in the next few days, if I don't find any bugs in real use then it will be ready to merge from my viewpoint. Feel free to review now if anyone wants to. |
|
@marcus7070 Thanks for all your work on this. I took a quick look through the changes and they look good. I've added myself and @adam-urbanczyk as reviewers. When your testing is done, please let us know. |
|
@marcus7070 How is your testing going on this? |
|
@jmwright I was busy over the new year period but just picked this up again yesterday actually. I'm doing a rewrite on a large (and messy) project using this branch. Will be a good chance to not just test it's stability but to also make sure it's easy to use. I'll finish this in a few days and mark this PR ready for review and merge. |
|
Would there be any interest in a import cadquery as cq
part = cq.Workplane("XY").box(3, 3, 3)
negative_part = part.faces(">Z").sphere(1, combine=False)
part = part.cut(negative_part)In what I'm proposing: part = (cq.Workplane("XY").box(3, 3, 3)
.tag("base").faces(">Z").sphere(1, combine=False).subtract_from_tagged("base"))At the moment I am going to leave The obvious suggestion is to extend the existing Lines 2467 to 2472 in 74573fc
cut has the negative shape in the arguments and cuts from the current solid, subtract_from_tagged is the other way around, the negative shape is the current solid.
|
|
@marcus7070 I like it, but I wonder if it shouldn't be in a separate PR. With regards to not wanting to bloat CadQuery, I suppose |
|
@marcus7070 @jmwright I feel like CQ needs this functionality, but a subtract_fom_tagged doesnt feel right. It feels like this use case should be core product. Combined with tags, this presents an opportunity to really enhance CQ. the solution that 'feels' right to me would be to use the cut() method, but we need to think about more flexible abstractions to handle the scope of operations. At an abstract level, the problem we have here is that CQ operations only support one modelling context for each chain, AND cq operations have no notion of the 'scope' of an operation. The result is that its odd to create some objects, and then subtract them later on Solidworks handles this by providing a 'scope' choice on every operation. so while there is one big feature tree, each cut operation can choose to operate on the entire tree, or only on particular objects. We could imagine that operations today with no scope/tag are using a special tag 'all' Translating that way of working to CQ, you'd get something like this: The box in line 1 is created in the 'all' scope/tag, since none is provied. This way of working would be backwards compatible with existing code, but would free up authors to create arbitrary combinations of objects that are then managed much more flexibly. What do you guys think? |
|
@dcowden thanks for taking the time to write that out so clearly. The "scope" term always confused me. Basically it's just having multiple bodies and selecting which body/bodies the operation applies to, correct? I've written so many different replies here and deleted them all because clearly I need to think about this more. I'd love to implement this stuff, either in this PR or another depending on how closely it ties in with what I've already done. One thing I do want to say though is that at the moment a tag refers to the first instance in the parent chain with that tag. Actually, I think I'll try to cram the code of Lines 86 to 95 in 4fd5784
Lines 265 to 279 in 4fd5784
If I make tag default to all and have it represent some kind of pseudo-body then I'll lose some of that simplicity. At least I think I would, it's kind of hard to know before you write the code.
|
|
@marcus7070 @dcowden I will review this PR in more detail later but for now I can state the following: I'm personally not in favor of making things too convoluted (e.g. introducing implicit tags and in-place CQ stack modification). IMHO the CQ stack should be immutable (or rather append only). Otherwise debugging of complicated models will be really difficult. Going with the sphere cut example I don't understand what is not OK with the current way of doing such a cut. base = cq.Workplane("XY").box(3, 3, 3)
sphere = base.faces(">Z").sphere(1,combine=False)
result= part.cut(sphere)BTW there is also #30 which, if implemented, would result in: base = cq.Workplane("XY").box(3, 3, 3).faces(">Z").sphere(1,subtract=True) |
|
Those are good points @adam-urbanczyk. Indeed, I agree that introducing stack mutability would create confusion. we don't want to do that. I would imagine that introducing tag scope for operations would require making the stack have a list of objects, rather than just one. All current operations would act on all objects by default. Reverse compatibility would mean untagged operations keep modifying the first entry in the list. The essence of your main point still stands: is this complexity merited simply to save one line of code? Today, dealing with multiple objects can be accomplished by creating separate stacks, and then using plain old code to interact between them, and there's nothing wrong with that. Fluency alone doesn't merit a change of this complexity. My motivation is really based upon the power of selectors. Currently, all selectors are based on geometry ( or logical combinations of geometry). That means that you can always take one CQ object, and start a new one using the original object, and ALL selectors will work on that new stack vs the old one. Tag based selectors changes things. Tags are stored in the stack, and you lose those tags ( and their object references ), when you make a new stack. This means that in addition to losing fluency, you potentially lose the ability to use tags that were generated earlier as a part of operations in the new stack. In the simple example contemplated here, the sphere is subtracted from the top face, and getting to it is user easy using ">Z". But as an example, suppose you tagged the top of an object, and then did a bunch of other operations, and then want to subtract something using that face as a reference later on. In one stack you can do that, and use tags to help you 'find' geometry references in prior layers of the stack. But with two stacks, that is lost. Or maybe it isnt? Perhaps we could just provide a way to copy a whole stack, including tags? Then this argument vanishes. |
|
There is a lot of things for me to look over here, but I've only got a few minutes so I just quickly renamed To do:
|
There is nothing broken about it and it achieves the correct result. Like you said in #30, it would just be nice to do it with a chained call. If you are going to do Boolean operations in a chained call then you need some method to specify the two solids, hence why I'm thinking about it here with tags. But perhaps there is a method that is more in line with existing CadQuery code. The current Lines 2467 to 2496 in 74573fc
what about if toCut is None:
toCut = self.findSolid(searchStack=True, searchParents=False)
solidRef = self.findSolid(searchStack=False, searchParents=True)So that if someone calls part = (cq.Workplane("XY").box(3, 3, 3)
.faces(">Z").sphere(1, combine=False).cut(None))If that is the way to go, then that code would be for a different PR and I can forget about it for a while. |
I keep getting confused here. How come you guys refer to the parent chain as the stack, when the docstrings use the term "stack" to refer to the |
|
@marcus7070 there could be indeed some inconsistencies in the wording. NB: I'm personally in favor of the #30 syntax: part = (cq.Workplane("XY").box(3, 3, 3)
.faces(">Z").sphere(1, subtractive=True)over your proposal part = (cq.Workplane("XY").box(3, 3, 3)
.faces(">Z").sphere(1, combine=False).cut(None))I've seen the subtractive features concept in FreeCAD so I guess the idea itself shouldn't be totally weird. |
b415338 to
04c8b80
Compare
|
Do you want to review @dcowden or shall we merge as is? |







In implementing this feature I've had to touch some pretty fundamental CQ code, so I'm going to write a detailed description of what I've been up to here, because I'd really like others to follow along, provide good feedback, and hopefully get this merged without treading on other's toes.
Tags
The main feature I'm implementing is the abilty to tag an object (method
CQ.tag) within the chain, and reload that object at a later point (methodCQ.getTagged).I've also created a method to create a workplane in the current CQ object by copying the workplane from another CQ object (method
CQ.copyWorkplane). This is combined with tags in methodCQ.copyWorkplaneFromTagged, allowing things like:A good example of a use case is modelling a PCB.
Construction geometry using tags
CadQuery shares the modelling context between all objects in the chain. This has an interesting interaction with tags, in that you can easily:
getTagged,This could be very useful when step 2 involves creating complicated geometry, and you can't be bothered doing the maths to figure out where the edges need to go in step 3.
I wrote a test for this use case if you want to read some example code.
Other changes
This process of returning to tagged objects is a bit of an edge case for the existing CadQuery code, so I had to make a couple of changes to handle some errors that popped up.
Workplane.newObjectpreviously referenced the parent's plane. This madecopyWorkplaneFromTaggedproduce some very confusing results, where the plane of a tagged object could be modified by objects further down the chain. I changednewObjectto copy the plane instead. I acknowledge this increases the memory requirements, but I think it's worth it and alsonewObjectis now more in line with how the user would expect it to function.Workplane.centerpreviously changed the current object's plane before creating a new object with the current object's plane. I changed it such that the current object's plane is not modified. It's not as neat, but I think it's how users expectcenterto function.testWorkplaneCenterin test_selectors previously relied upon the mutable plane. The style of that test didn't seem to follow CadQuery's chained calls, so I changed it. I don't understand why the original code was written in that style, so I'm bringing attention to it here in case I messed with something I shouldn't. I see that test was some of the earliest CQ2.0 code written, and perhaps that was why it didn't use chained calls?Todo
PS. I'm loving CadQuery. It's a light layer over OCC, so it's just so flexible. Extending it like this has been really enjoyable. None of that "smashing my head against the keyboard" as I struggle against heavy, opaque code I can't understand.