-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add MTL conversion unit tests #388
Conversation
Ensures that the behavior is effective and avoids edges cases.
Had to do this transform since we don't want to install / uninstall color spaces for tests. Also, pivoted to return early and return None if nothing processed, so we can test for that too.
"# Thanks c:\n" | ||
) | ||
# This represents a new folder that'll backup the MTL filepath | ||
original_mtl_path = Path(filepath).parent.absolute() / "ORIGINAL_MTLS" |
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.
@StandingPadAnimations is there a reason to use this over something more like:
original_mtl_path = Path(filepath).parent.absolute() / "ORIGINAL_MTLS" | |
original_mtl_path = Path(os.path.join(filepath, "ORIGINAL_MTLS")).parent.absolute() |
I just worry about the slash being OS dependent. But if it worked for you (on a windows machine?) then I'm good with it.
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.
And I did test on 2.7x, works fine.
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.
Well I tested my change above, and it actually breaks the unit tests (a sign they are working), not sure exactly why though, I'm also just less familiar with the Path module.
So, really the main thing is if you could confirm you were testing on windows, then I'm all set.
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'm on Linux but the Python documentation says that it should be fine.
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.
Thanks for the doc reference, good to know. Unless you have any particular complaints here, I'll go ahead and merge in a moment.
I just need to check the Blender C++ OBJ code to see if indeed MTL conversion is needed for 3.5 and above |
Alright, I don't see any mention of "Non-Color Data" anywhere, so it's hard for me to determine if indeed the C++ OBJ importer has an intended behavior change. For now I think it's somewhat safe to say that it's an oversight, but I can't be certain |
Will need to rebase once #383 is merged, as that is the base for this branch.
These tests are being added to add extra sureness that the expected changes are happening when editing an mtl file.