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

fix: Transform order in subassembly #1629

Merged

Conversation

lenianiva
Copy link
Contributor

@lenianiva lenianiva commented Jul 14, 2024

Fixes #1628

Ideally I want to add some unit tests but I don't know the best way to approach this. Maybe test for the intersection volume of the black and white arrows?

Copy link

codecov bot commented Jul 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.98%. Comparing base (edabe5e) to head (8cc125e).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
+ Coverage   94.92%   94.98%   +0.06%     
==========================================
  Files          28       28              
  Lines        6263     6322      +59     
  Branches     1271     1292      +21     
==========================================
+ Hits         5945     6005      +60     
  Misses        192      192              
+ Partials      126      125       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adam-urbanczyk adam-urbanczyk self-requested a review July 16, 2024 06:46
@adam-urbanczyk
Copy link
Member

Thanks @lenianiva ! As you mentioned in #1628 you could add a test asserting if the sublocs are equal.

@lenianiva
Copy link
Contributor Author

lenianiva commented Jul 16, 2024

Thanks @lenianiva ! As you mentioned in #1628 you could add a test asserting if the sublocs are equal.

I think testing exact equality doesn't work. I added a really simple example which has different behaviours under the old and new implementations, and the coordinates don't fully match up.

import cadquery as cq
from functools import reduce
from typing import Tuple, cast

def f():
    cube = cq.Solid.makeBox(1, 1, 1)
    inner = (
        cq.Assembly()
        .add(cube, name="c1")
        .add(cube, name="c2", loc=cq.Location((0, 0, 1), (0, 1, 0), 45))
    )
    middle = (
        cq.Assembly()
        .add(inner, name="inner")
    )
    outer = (
        cq.Assembly()
        .add(middle, name="middle", loc=cq.Location((0,0,0)))
    )


    #cq.Assembly._subloc = _subloc
    name, _ = middle._query("inner/c2")
    native_loc = middle.objects[name].loc
    print(native_loc.toTuple())
    loc, _ = middle._subloc(name)
    print(loc.toTuple())


    name, _ = outer._query("middle/inner/c2")
    native_loc = outer.objects[name].loc
    print(native_loc.toTuple())
    loc, _ = outer._subloc(name)
    print(loc.toTuple())

f()

@lenianiva
Copy link
Contributor Author

@adam-urbanczyk I don't think using approx is the issue here, but the location from _query and _subloc each encode a part of the relative position of the referenced anchor. e.g. the following generates the absolute location of an object

    result = ... # some assembly
    key = "assembly/obj?tag"
    name, sh = result._query(key)
    loc_self = sh.location()
    loc_parent, _ = result._subloc(name)
    loc = loc_parent * loc_self
    print(name, loc.toTuple())

@adam-urbanczyk
Copy link
Member

Yup, trying to fix the test. I distilled the example made by @lorenzncode :
afbeelding
Those two spheres should coincide after the fix, and this is exactly what the test asserts now.

Copy link
Member

@lorenzncode lorenzncode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one typo in test comment.

tests/test_assembly.py Outdated Show resolved Hide resolved
@adam-urbanczyk
Copy link
Member

Thanks @lenianiva and @lorenzncode !

@adam-urbanczyk adam-urbanczyk merged commit 3cd327a into CadQuery:master Jul 25, 2024
5 checks passed
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.

[Bug] Incorrect transform chaining in nested assemblies
3 participants