Implemented MTL conversion for ACES and whatnot #383

StandingPadAnimations · 2023-03-08T22:52:07Z

ACES and other colorspaces like it have issues when it comes to OBJ importing, so we need to convert the MTL so that Blender doesn't throw an exception when importing.

This comments out lines starting map_d from the MTL. The operation itself shouldn't affect MCprep since we don't deal with it anyway.

ACES and other colorspaces like it have issues when it comes to OBJ importing, so we need to convert the MTL so that Blender doesn't throw an exception when importing. This comments out lines starting `map_d` from the MTL. The operation itself shouldn't affect MCprep since we don't deal with it anyway. Of course the backup needs to be fixed (right now it will always back up the file, so we could add a timestamp to the edited MTL that MCprep then checks for and compares before attempting to convert)

TheDuckCow

Change looking good, I'll want to live test once you make the requested change below to push it into a function.

Always a good idea to make small functions that do very specific things, and avoid nesting (blocks, try/catch etcs) as much as possible. But this will be a very welcome change (also thanks for splitting up these two PRs, feels much cleaner this way).

MCprep_addon/world_tools.py

StandingPadAnimations · 2023-03-09T18:46:18Z

There is one small thing I need to implement as well: caching

Right now this branch will always copy the MTL to the backup folder, but it doesn't convert the MTL back to its original form. I'm thinking of adding a small "header" at the end of the edited MTL that says the following:

# This section was added by MCprep's MTL conversion script in order to allow importing without an
# error being thrown
# <timestamp of copied MTL>

The MTL conversion code was added to a seperate function so that we don't have to deal wtih 2 try-except blocks in the same place. It also gives more flexability in how we approch conversion

Beyond just not having to perform a useless operation (useful on larger MTLs), it also prevents edited MTLs from being copied into the ORIGINAL_MTLS directory, which is important as it keeps the original MTL intact. This is done by simply adding a header at the end of the MTL file whose existance is then checked. The reason we do it at the end of the file is because: - It's easier - It's less likely to interfere with whatever header a world exporter may create in the MTL file At this point, I think it's safe to say that we're ready for review.

StandingPadAnimations · 2023-03-10T05:49:50Z

Just adding this for tommorow me:

Add MTL recovering (since we can reverse the operations MCprep does to convert MTLs)
Simplify header checking (inatead of list comprehension, we should just check the last 3 lines since MCprep will always write the header as the last part of the MTL)

TheDuckCow

Sorry for all the nit picks, but it's getting there.

As I said, let me know if you have any interest in making the test *function, otherwise I can do it.

TheDuckCow · 2023-03-10T13:51:19Z

MCprep_addon/world_tools.py

@@ -87,6 +87,64 @@ def detect_world_exporter(filepath):
 	return 'jmc2obj'


+def convert_mtl(filepath):


One more thing - can you upload an example of your intended modified, and the corresponding original file?

I'd like to make a unit test that validates this happens, so we can know this continues to work in the future (I'm happy to do that work, unless you wanted to take a stab at it; I'd be saving both files to the test directory and using the original to then bytewise compare the other in a temp space)

MTL conversion simply does the following:

Comment out lines that begin with map_d

Add a header at the end

Here's an original MTL and the edited MTL

Thanks a bunch. See the followup PR I've created which includes these test files:
#388 (diff will be reduced to mostly just the test file once this PR 383 is merged)

MCprep_addon/world_tools.py

Formatting is now based on PEP8 guidlines

Exceptions are also printed now

The copy operation now performs in a try-except loop for exception handling

Moved call to convert_mtl outside of try-except (because it already has exception handling internally) and changed the reporting to a warning (since it shouldn't affect execution)

All variables should now follow PEP8

Since MCprep will always output its header at the end of the MTL, we can simply check the last 3 lines of the MTL, which removes an unnecesary for loop, and makes the code easier to read.

If an exception occurs when writing to the file, we want to gracefully recover, hence the copying the original MTL back. TODO: Confirm if `mtl` will be an absolute path

StandingPadAnimations · 2023-03-11T23:14:17Z

@TheDuckCow Made the necessary changes, although some guarantees may need to be added (like will self.filepath always be absolute?). Other then that, I think this is for the most part done

TheDuckCow · 2023-03-12T05:33:23Z

FYI @StandingPadAnimations of a minor merge conflict resolved, in case you do further work that you'll need to pull first. Reviewing shortly!

TheDuckCow

Approving this.

Ready to merge, and then I'd move on to look at my PR followup here (don't review until this PR has been merged though; I couldn't use yours as a base since it's a different repo).

Also, is it fair to say that this change is actually somewhat negated now that we are moving to the new C++ obj importer in blender 3.5? I realize it'll still be helpful for the 3.4 and older users, but I just wanted to make sure I am aware.

TheDuckCow · 2023-03-12T05:40:22Z

MCprep_addon/world_tools.py

@@ -87,6 +87,64 @@ def detect_world_exporter(filepath):
 	return 'jmc2obj'


+def convert_mtl(filepath):


Thanks a bunch. See the followup PR I've created which includes these test files:
#388 (diff will be reduced to mostly just the test file once this PR 383 is merged)

TheDuckCow · 2023-03-12T20:41:40Z

MCprep_addon/world_tools.py

+	with open(mtl, 'r') as mtl_file:
+		lines = mtl_file.readlines()
+
+	if bpy.context.scene.view_settings.view_transform not in blender_standard:


FYI only in my branched, I inverted this logic and returned early, cutting out the amount of indention below.

There it also returns "None" to indicate no conversion happened, which is a useful state imo.

StandingPadAnimations · 2023-03-12T21:18:07Z

Also, is it fair to say that this change is actually somewhat negated now that we are moving to the new C++ obj importer in blender 3.5? I realize it'll still be helpful for the 3.4 and older users, but I just wanted to make sure I am aware.

From what I can tell, the behavior of the new C++ importer is either an oversight or now it checks for the existance of whatever colorspace it wants to use. As such, it's also good to fix the root of the issue, just in case

StandingPadAnimations requested a review from TheDuckCow March 8, 2023 22:52

This was linked to issues Mar 8, 2023

Blender ACES unable to import OBJ #231

Closed

ACES Colorspace Support #330

Closed

StandingPadAnimations removed a link to an issue Mar 8, 2023

ACES Colorspace Support #330

Closed

1 task

StandingPadAnimations self-assigned this Mar 8, 2023

TheDuckCow requested changes Mar 9, 2023

View reviewed changes

MCprep_addon/world_tools.py Outdated Show resolved Hide resolved

StandingPadAnimations added 2 commits March 9, 2023 12:55

Moved MTL conversion code to seperate function

7ac92f6

The MTL conversion code was added to a seperate function so that we don't have to deal wtih 2 try-except blocks in the same place. It also gives more flexability in how we approch conversion

TheDuckCow requested changes Mar 10, 2023

View reviewed changes

StandingPadAnimations added 3 commits March 10, 2023 10:22

Fixed formatting

068ac00

Formatting is now based on PEP8 guidlines

Minimized try-except block

320fb54

Exceptions are also printed now

Added try-except around copy operation

9a067dd

The copy operation now performs in a try-except loop for exception handling

StandingPadAnimations force-pushed the mtl-conversion branch from 43579c9 to 9a067dd Compare March 10, 2023 16:45

StandingPadAnimations added 5 commits March 10, 2023 10:45

Some changes

d98914d

Moved call to convert_mtl outside of try-except (because it already has exception handling internally) and changed the reporting to a warning (since it shouldn't affect execution)

Fixed variable formatting

f71d874

All variables should now follow PEP8

Changed how header is checked

f05ff07

Since MCprep will always output its header at the end of the MTL, we can simply check the last 3 lines of the MTL, which removes an unnecesary for loop, and makes the code easier to read.

Added a comment explaning the conversion

9285f98

Added MTL recovery if exception occurs

56d3d4a

If an exception occurs when writing to the file, we want to gracefully recover, hence the copying the original MTL back. TODO: Confirm if `mtl` will be an absolute path

Merge branch 'dev' into mtl-conversion

91976f4

TheDuckCow mentioned this pull request Mar 12, 2023

Add MTL conversion unit tests #388

Merged

TheDuckCow self-requested a review March 12, 2023 20:40

TheDuckCow approved these changes Mar 12, 2023

View reviewed changes

TheDuckCow merged commit a48820f into Moo-Ack-Productions:dev Mar 12, 2023

StandingPadAnimations deleted the mtl-conversion branch March 13, 2023 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented MTL conversion for ACES and whatnot #383

Implemented MTL conversion for ACES and whatnot #383

StandingPadAnimations commented Mar 8, 2023

TheDuckCow left a comment

StandingPadAnimations commented Mar 9, 2023

StandingPadAnimations commented Mar 10, 2023

TheDuckCow left a comment •

edited

Loading

TheDuckCow Mar 10, 2023

StandingPadAnimations Mar 10, 2023

TheDuckCow Mar 12, 2023

StandingPadAnimations commented Mar 11, 2023

TheDuckCow commented Mar 12, 2023

TheDuckCow left a comment

TheDuckCow Mar 12, 2023

TheDuckCow Mar 12, 2023

StandingPadAnimations commented Mar 12, 2023

		@@ -87,6 +87,64 @@ def detect_world_exporter(filepath):
		return 'jmc2obj'


		def convert_mtl(filepath):

Implemented MTL conversion for ACES and whatnot #383

Implemented MTL conversion for ACES and whatnot #383

Conversation

StandingPadAnimations commented Mar 8, 2023

TheDuckCow left a comment

Choose a reason for hiding this comment

StandingPadAnimations commented Mar 9, 2023

StandingPadAnimations commented Mar 10, 2023

TheDuckCow left a comment • edited Loading

Choose a reason for hiding this comment

TheDuckCow Mar 10, 2023

Choose a reason for hiding this comment

StandingPadAnimations Mar 10, 2023

Choose a reason for hiding this comment

TheDuckCow Mar 12, 2023

Choose a reason for hiding this comment

StandingPadAnimations commented Mar 11, 2023

TheDuckCow commented Mar 12, 2023

TheDuckCow left a comment

Choose a reason for hiding this comment

TheDuckCow Mar 12, 2023

Choose a reason for hiding this comment

TheDuckCow Mar 12, 2023

Choose a reason for hiding this comment

StandingPadAnimations commented Mar 12, 2023

TheDuckCow left a comment •

edited

Loading