fix eachpoint() with useLocalCoordinates=False coordinate transformation

[greyltc]

I think the coordinate transformation for the eachpoint() function with useLocalCoordinates=False was backwards
fixes #1098

[codecov]

Codecov Report

Merging #1097 (ae2db62) into master (a5fadeb) will not change coverage.
The diff coverage is 100.00%.

❗ Current head ae2db62 differs from pull request most recent head 5881c36. Consider uploading reports for the commit 5881c36 to get more accurate results

@@           Coverage Diff           @@
##           master    #1097   +/-   ##
=======================================
  Coverage   96.34%   96.34%           
=======================================
  Files          40       40           
  Lines        9533     9533           
  Branches     1259     1259           
=======================================
  Hits         9185     9185           
  Misses        205      205           
  Partials      143      143           

Impacted Files	Coverage Δ
cadquery/cq.py	92.43% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[jmwright]

@greyltc Is there an example that shows how it was wrong?

[greyltc]

@greyltc Is there an example that shows how it was wrong?

I kinda hoped you'd look at this and say "oh yeah" ;-)

I do have an example (from when I ran into this the other day), but it's a long way from being minimal. I'll see if I can put something minimal together. Is there any case anywhere eachpoint(...,useLocalCoordinates=False) works? Could it be that be bug only appears when there's rotation involved in the transformation?

[greyltc]

@jmwright, hopefully that ^^ issue report makes it clear!

[jmwright]

I tried a few things with this change and I think the fix makes sense. It seems odd that this bug would have gone undiscovered for so long, so that made me suspicious that the way it was working was just counter-intuitive and not broken.

@gumyr Have you run into this bug before while working on cq_warehouse? Here's the accompanying issue: #1098

[gumyr]

Interestingly, I only use eachpoint once in all of cq_warehouse - in my sprocket generator (one of my oldest designs) - and I had to make a change on May 11 to adapt to #1016 where polarArray feed points into that eachpoint call. Either I don't directly see the problem, or I thought that's how it should be and adapted.

…

On Tue, Jun 7, 2022 at 6:56 AM Jeremy Wright ***@***.***> wrote: I tried a few things with this change and I think the fix makes sense. It seems odd that this bug would have gone undiscovered for so long, so that made me suspicious that the way it was working was just counter-intuitive and not broken. @gumyr <https://github.com/gumyr> Have you run into this bug before while working on cq_warehouse? Here's the accompanying issue: #1098 <#1098> — Reply to this email directly, view it on GitHub <#1097 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUNL25QOJX4F4WSXEXPO7HLVN4THDANCNFSM5X47DMYQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[greyltc]

I tried a few things with this change and I think the fix makes sense. It seems odd that this bug would have gone undiscovered for so long, so that made me suspicious that the way it was working was just counter-intuitive and not broken.

@gumyr Have you run into this bug before while working on cq_warehouse? Here's the accompanying issue: #1098

I think nobody's ever seen this because eachpoint(...,useLocalCoordinates=False) is never used. I haven't been able to find any instance where eachpoint() is used in the wild with useLocalCoordinates=False. Everyone seemingly always calls it with useLocalCoordinates=True, which does not run the line that his PR fixes.

[jmwright]

Thanks @greyltc

[jmwright]

@adam-urbanczyk @lorenzncode Any objections to merging this?

[lorenzncode]

@jmwright I would suggest not to merge this. There doesn't seem to be agreement on a description of the issue.

Here is a simpler example tested with master that looks correct to me.:

# global coordinates
pts = [
    (0, 20, 40),
    (10, 20, 40),
    (0, -20, 40),
]

cyl = cq.Solid.makeCylinder(2, 8, (0, 0, -4))

r = (
    cq.Workplane("XZ")
    .pushPoints(pts)
    .eachpoint(
        lambda loc: cyl.moved(loc),
        #useLocalCoordinates=True,
        useLocalCoordinates=False,
    )
)

With useLocalCoordinates=False, the cylinder centers are the same as pts.:

cq-editor console:

%precision 0
Out[1]: '%.0f'

[s.Center().toTuple() for s in r.solids().vals()]
Out[2]: [(-0, 20, 40), (10, 20, 40), (-0, -20, 40)]

The normal direction is also as expected (IMO).

r.faces("%Plane").val().normalAt()
Out[3]: Vector: (0.0, -1.0, 0.0)

When I test with this PR and again useLocalCoordinates=False I get:

[s.Center().toTuple() for s in r.solids().vals()]
Out[3]: [(-0, -40, 20), (10, -40, 20), (-0, -40, -20)]

[adam-urbanczyk]

@adam-urbanczyk @lorenzncode Any objections to merging this?

We did not reach a conclusion on this topic, so -1 on merging as-is.

[greyltc]

I think the root issue is #1099
This PR and the associated issue were my attempt at making things as consistent as I could in the case that root issue isn't solved.

So my favorite solution is to merge #1100 which obsoletes both issue #1098 and this PR