-
Notifications
You must be signed in to change notification settings - Fork 18
Extract Layout: Code restructure for readability and using actual containers as detected by integration #386
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
Extract Layout: Code restructure for readability and using actual containers as detected by integration #386
Conversation
|
@antirotor @moonyuet can you let me know what you think of these code changes to clean this up? This also fixes #121 (although purely that fix would've been smaller) but a lot of the code was quite unclear what it tried to do. This PR does remove the usage of the |
@antirotor would know a bit more about the use case. I usually turn this off and publish. |
|
I tested with publishing layout with this PR, it is successful but. It might be helpful if you can take a look at the data below the latest version is published from layout extractor in this branch |
|
Thanks @moonyuet - I fixed the matrix basis being integers, now they are floats again. I didn't care much about the empty Also, I'm quite surprised it actually worked and this is the only difference in the data 🤯 |
…avoid-hard-to-find-bugs
antirotor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the groupLoadedAsset - it is also set in Create Multishot Layout but used only here. I am ok to remove it and observe. In case it will be missed by someone, lets clearly define what it should do, implement and document it again.
| import re | ||
| import uuid | ||
| from typing import List | ||
| import attr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we don't need that anymore. We can use pure dataclasses, no? I think the idea was to eventually get rid of this dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we don't need that anymore. We can use pure dataclasses, no? I think the idea was to eventually get rid of this dependency.
Unfortunately - I think we'd then face that pyblish mixed with dataclasses and from __future__ import annotations issue, right? That's why I went for attr here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed already in pyblish-base - pyblish/pyblish-base#403
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.... ah but not released! 😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this as is @antirotor - I think we can improve later if need be.
LiborBatek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me...both .ma and .json been working without any glitches.
LGTM
…avoid-hard-to-find-bugs
Changelog Description
Extract Layout: Code restructure for readability and using actual containers as detected by integration
Additional review information
Still draft
Fix #121
Testing notes: