-
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
Fix MTL conversion bug with Mineways MTLs #397
Fix MTL conversion bug with Mineways MTLs #397
Conversation
There's been a issue recently where Mineways will name MTLs weirdly. For instance: - "Beach Town.obj" : "Beach_Town.mtl" - "hello there.obj" : "hello_there.mtl" The MTL conversion function simply replaces the extension of self.filepath, assuming that the MTL has the same name. However, since Mineways adds underscores, convert_mtl would throw an exception when attempting to read the file. The current workarounds are to: - Remove the underscores - Don't add spaces to OBJ names (ideally no one should be adding spaces to any filename because of issues like this) - Use jmc2OBJ, which as far as I know doesn't have this weird behavior This is more of a band-aid, and ideally we should check for underscores in the MTL name (we can take advantage of pathlib for this), but for now if an exception occurs due to a weirdly named MTL, we'll just print the error, return False, and call it a day.
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.
Hey, am I reading this right, that this would be causing complete import errors for a large swath of users? If so, we might need to do an emergency patch release. Unfortunately my error analytics pipeline slightly broke so we're flying blind for a couple days until I fix that (cloud related issue only).
I also should have thought to error handle that and later lines.
By taking advantage of the pathlib module (Seriously, why don't we use it more? It makes file operations so much easier and is compatible with the os module), we check to see if the assumed MTL path is valid. If not, we then replace the spaces in the MTL filename with underscores. If still not, then we return False. This assumes that an MTL will always have either the same name as the corresponding OBJ, or that Mineways went ahead and added underscores.
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.
Didn't live test with a model, but all unit tests are passing on this branch. Good to merge imo, and will continue to plan for another release next week as scheduled.
There's been a issue recently where Mineways will name MTLs weirdly. For instance:
The MTL conversion function simply replaces the extension of self.filepath, assuming that the MTL has the same name. However, since Mineways adds underscores, convert_mtl would throw an exception when attempting to read the file. The current workarounds are to:
spaces to any filename because of issues like this)
behavior
This is more of a band-aid, and ideally we should check for underscores in the MTL name (we can take advantage of pathlib for this), but for now if an exception occurs due to a weirdly named MTL, we'll just print the error, return False, and call it a day.