Skip to content
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

Using the C++ OBJ importer and Fixing Issues with Importing when using a Non-standard colorspace #373

Conversation

StandingPadAnimations
Copy link
Collaborator

This comes at the cost of having a split OBJ, but it solves 2 issues:

  • Performance
  • Errors involving colorspaces like ACES and AgX

Perhaps we could put this behind a user prefs option, so users can use the legacy importer if they need to (though idk what stops a user from going into edit mode, selecting the OBJ, and splitting by material)

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented Feb 16, 2023

Ok we should start weighing the tradeoffs of using the C++ OBJ importer for 3.1 - 3.4, since feature wise the C++ importer loses a major feature (split groups) that was only added in 3.5

@StandingPadAnimations
Copy link
Collaborator Author

I think I'll keep it enabled in 3.1 - 3.4. The loss of functionality is made up for by the massive performance improvements.

I've noticed with even small OBJs the performance goes from seconds to instant imports, so who knows how this scales

@TheDuckCow
Copy link
Member

That's really awesome to hear. In terms of splitting the world back into by material, we could just run that function on import (that was the actual reason for using an custom operator for import, in the early days of 2.8 when everything was joined together). It will bring back a little bit of that slowness, but might still be worth it.

Thanks a ton for this, will try to get to review this. The imminent release urgency dropped as my tutorial sequence is not blocked on the need to fix sync materials (that still should be fixed tho). Will try to review this and your other change soon.

Copy link
Member

@TheDuckCow TheDuckCow left a comment

Choose a reason for hiding this comment

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

Largely good, but worried about editing mtl's and one reversion for 3.4 branch.

MCprep_addon/world_tools.py Outdated Show resolved Hide resolved
MCprep_addon/world_tools.py Outdated Show resolved Hide resolved
MCprep_addon/world_tools.py Outdated Show resolved Hide resolved
MCprep_addon/world_tools.py Outdated Show resolved Hide resolved
When MCprep opens the file picker, both OBJs and MTLs can be selected,
which was annoying when accidently selecting the MTL file of the OBJ.

This adds a quality of life improvement by replacing .mtl with .obj if
the user accidently clicks the MTL file
MTL files need to be edited in order for OBJ importing to work properly
when using a non-standard colorspace like ACES or AgX. Of course,
editing MTL files and then reverting the edits can be error prone, and
if done wrong the user won't be able to retrieve the original MTL.

This can be fixed by creating a new folder called "ORIGINAL_MTLS" with
pathlib, and then copying the MTL to that new folder before editing. Now
that the original is backed up, we don't need to revert edits.

Of course the editing section will still run regardless of there's edits
or not, so we'll need to look into checking if an edit was made (perhaps
by comparing timestamps).
@StandingPadAnimations
Copy link
Collaborator Author

Alright, MTL files should now be copied to a new folder called ORIGINAL_MTLS before they're edited

@StandingPadAnimations
Copy link
Collaborator Author

Weird situation: Not sure why but somehow Kaion's master branch was merged with support-all-color-management. I'm looking into the issue, but I can't find the PR that somehow did it

@StandingPadAnimations
Copy link
Collaborator Author

Ight, I still haven't found the reason for this, so sadly I'll have to close this for now

Seems like Github did something to the remote repo that caused this issue

@StandingPadAnimations
Copy link
Collaborator Author

Ok so turns out when I forced reverted the merge commit, it didn't properly revert the merge. I'll look into reverting the force push (Github thankfully stored the commit in question)

@TheDuckCow do you know how to properly undo a merge?

This reverts commit 437749f, reversing
changes made to d13045e.

This was purely accidental, no idea what caused this. Hopefully this
fixes it now
@StandingPadAnimations
Copy link
Collaborator Author

Ok so I've reverted the merge. Sadly the history remains but oh well. I'll continue to see if I can remove the extra commits that were introduced, but hopefully this should be fine now

@StandingPadAnimations
Copy link
Collaborator Author

Good news, I've split this into 2 branches, and with some cherrypicking was able to move some commits over. I still need to reimplement MTL conversion but #382 now tracks support for the new C++ OBJ importer

@StandingPadAnimations
Copy link
Collaborator Author

Alright, I've now restored both changes, and they have their own PRs

@StandingPadAnimations StandingPadAnimations deleted the support-all-color-management branch March 8, 2023 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ACES Colorspace Support Blender ACES unable to import OBJ
2 participants