CadQuery / cadquery

A python parametric CAD scripting framework based on OCCT
https://cadquery.readthedocs.io
Other
3.03k stars 284 forks source link

`eachpoint(...,useLocalCoordinates=False)` has a coordinate transformation bug #1098

Open greyltc opened 2 years ago

greyltc commented 2 years ago

For example, the output of

import cadquery
from cadquery import cq, CQ

mwp = CQ("ZX").circle(80.0).extrude(12)
mwp = mwp.translate((99, 99, 99))
mwp = mwp.faces("<Y").workplane(centerOption="CenterOfBoundBox")

def minimal_plugin(wp: cq.Workplane) -> cq.Workplane:
    def minimal(loc: cadquery.Location):
        box = CQ().box(length=20, width=10, height=30, centered=False)
        return box.findSolid().moved(loc)

    return wp.eachpoint(minimal, useLocalCoordinates=False, combine=True)

cq.Workplane.minimal_plugin = minimal_plugin
mwp = mwp.rarray(1, 40, 1, 3).minimal_plugin()
show_object(mwp)

is (A) image but it should be (B) image

Luckily https://github.com/CadQuery/cadquery/pull/1097 fixes it!

lorenzncode commented 2 years ago

Try useLocalCoordinates=True. The first output is correct because the rarray points will be in XY in global coordinates.

greyltc commented 2 years ago

The above is a contrived example specifically to illustrate the bug, not to achieve any final geometry that I want.

@lorenzncode, are you aware of any usage of eachpoint(...,useLocalCoordinates=False) in the wild today where it's working as expected?

"global XY coordinates" are (0,0,0) at the center of the axis cross. (A) doesn't put the boxes there, so why is it correct?

adam-urbanczyk commented 2 years ago

Is it incorrect or not according to your expectations @greyltc ?

jmwright commented 2 years ago

@adam-urbanczyk @lorenzncode This issue is just meant to give an example of what is fixed by #1097

I believe @greyltc is saying that the current behavior does not match expectations.

greyltc commented 2 years ago

Is it incorrect or not according to your expectations @greyltc ?

I've tried to keep my expectations of how I think eachpoint(...,useLocalCoordinates=False) should work out of this for now. My PR and this issue are filed against how I think the original author intended eachpoint(...,useLocalCoordinates=False) to work.

I'll post a separate issue on how I believe there's a mismatch between what the documentation says and what the code actually does when eachpoint(...,useLocalCoordinates=False) is called to not muddy the waters here :-).

greyltc commented 2 years ago

I've made https://github.com/CadQuery/cadquery/issues/1099 which covers my expectations based on the docs and how each() works. If the PR associated with that one is merged, this issue and its PR are obsolete.

adam-urbanczyk commented 2 years ago

I'm not sure that the current implemntation is correct, but what you write contradicts with the screenshot @greyltc .

greyltc commented 2 years ago

what you write contradicts with the screenshot

@adam-urbanczyk You mean what I wrote in https://github.com/CadQuery/cadquery/issues/1099 contradicts what's here?

https://github.com/CadQuery/cadquery/issues/1099 is meant to be an entirely separate issue from this one (they just happen to occur on the same line). Solving https://github.com/CadQuery/cadquery/issues/1099 (in the way I imagine it should be solved, as in https://github.com/CadQuery/cadquery/pull/1100) makes this whole issue (and its solution) obsolete.

If you agree that https://github.com/CadQuery/cadquery/issues/1099 describes a real problem, then probably ignore this issue. If you disagree that https://github.com/CadQuery/cadquery/issues/1099 is a problem, then consider this issue.

lorenzncode commented 2 years ago

The rarray points are:

Vector: (99.0, 99.0, 59.0)
Vector: (99.0, 99.0, 99.0)
Vector: (99.0, 99.0, 139.0)

Maybe I got it wrong and @greyltc is correct to expect the boxes to vary in Z here.

adam-urbanczyk commented 2 years ago

On the contradiction: @greyltc you suggest in one of the comments that the objects should be centered around (0,0,0), but it is clearly not the case in the screenshot B. Could you give a definitive description of what is the desired behavior?

AFAICT screenshot A shows the correct behavior and B is just useLocalCoordinates=True.