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

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

Open
greyltc opened this issue Jun 5, 2022 · 10 comments · May be fixed by #1097
Open

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

greyltc opened this issue Jun 5, 2022 · 10 comments · May be fixed by #1097

Comments

@greyltc
Copy link
Contributor

greyltc commented Jun 5, 2022

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 #1097 fixes it!

@lorenzncode
Copy link
Member

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

@greyltc
Copy link
Contributor Author

greyltc commented Jun 6, 2022

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

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

@jmwright
Copy link
Member

jmwright commented Jun 6, 2022

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

greyltc commented Jun 8, 2022

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

greyltc commented Jun 8, 2022

I've made #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
Copy link
Member

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

@greyltc
Copy link
Contributor Author

greyltc commented Jun 8, 2022

what you write contradicts with the screenshot

@adam-urbanczyk You mean what I wrote in #1099 contradicts what's here?

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

If you agree that #1099 describes a real problem, then probably ignore this issue. If you disagree that #1099 is a problem, then consider this issue.

@lorenzncode
Copy link
Member

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

adam-urbanczyk commented Jun 10, 2022

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.

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 a pull request may close this issue.

4 participants