Skip to content

Add set_offset3d and get_offset3d methods to 3D Collections #27556

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

scottshambaugh
Copy link
Contributor

@scottshambaugh scottshambaugh commented Dec 21, 2023

PR summary

Addresses #784 in part

More discussion in #19573

Note that the inherited Collection.set_offset and Collection.get_offset methods with 2D coordinates are used in zordering calculations and I couldn't see an easy way to abstract those out in order to override those methods and keep the same names as the 2D case.

PR checklist

@scottshambaugh
Copy link
Contributor Author

scottshambaugh commented Jan 11, 2024

Only test failure is codecov, but these are fairly well covered and should be fine.

@scottshambaugh scottshambaugh added this to the v3.9.0 milestone Mar 1, 2024
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Minor style fixes / improvements.

@timhoffm
Copy link
Member

I took the liberty to create a commit with the style fixes.

@timhoffm
Copy link
Member

timhoffm commented Mar 11, 2024

Could you add a simple test? I think just manipulating the values and reading them back to assert that you get the new offesets is sufficient. Or, if you want to be a bit more fancy make an image comparison test where you create a collection in fig_test with offests_0 and then set_offsets3d(offsets_1) and compare that to fig_ref with directly set offsets_1. This could also be amended with zdir.

@scottshambaugh scottshambaugh marked this pull request as draft March 12, 2024 23:01
@scottshambaugh scottshambaugh removed this from the v3.9.0 milestone Mar 12, 2024
@scottshambaugh
Copy link
Contributor Author

Moving back to draft since I need to dig more into what Poly3DCollection is doing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants