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

MTL Conversion Bug with Mineways OBJs #393

Closed
StandingPadAnimations opened this issue Mar 14, 2023 · 6 comments · Fixed by #397
Closed

MTL Conversion Bug with Mineways OBJs #393

StandingPadAnimations opened this issue Mar 14, 2023 · 6 comments · Fixed by #397
Assignees
Labels
bug Known issue This is an existing MCprep behavior that, while not a bug, is known to be problematic for some users
Milestone

Comments

@StandingPadAnimations
Copy link
Collaborator

It seems like if a user names their OBJ anything with spaces (like Beach Town.obj), Mineways will export an MTL where the name has underscores (like Beach_Town.mtl). This causes the MTL conversion function to fail since the first thing it does is read the MTL file in.

The MTL filepath is recieved by replacing the file extension of the OBJ filepath. Seems like we need to implement some check for underscores in MTL names as well

@StandingPadAnimations StandingPadAnimations added bug Known issue This is an existing MCprep behavior that, while not a bug, is known to be problematic for some users labels Mar 14, 2023
@StandingPadAnimations StandingPadAnimations self-assigned this Mar 14, 2023
@TheDuckCow TheDuckCow added this to the v3.4.3 milestone Mar 15, 2023
@TheDuckCow TheDuckCow linked a pull request Mar 15, 2023 that will close this issue
@TheDuckCow TheDuckCow pinned this issue Mar 19, 2023
@TheDuckCow
Copy link
Member

I got my analytics reporting working again. As suspected, this is the culprit of a large quantity of bugs, see this overall uptick corresponding to the v3.4.2 release:

Screen Shot 2023-03-19 at 8 08 25 AM

And filtering just for v.3.4.2 (the above chart was overall), we get 700 reports. Apparently, 494 of them (70%) are coming from this issue (another 70 or 10% are coming from the fixed block spawning material issue).

Therefore, I will be seeking to release this update as quickly as possibly - today or tomorrow.

@TheDuckCow
Copy link
Member

I realized we already merged this change @StandingPadAnimations but could we add one more precaution?

Could we actually skip overwriting the file unless we specifically know we have a pending modified a line? This way, we aren't unnecessarily modifying it for users who don't have the aces issue anyways (even if yes, it means having to reparse the entire MTL each time). Parsing the mtl should be generally speaking quite fast (at least not much slower than the type it takes to open the file to check for the MCprep header).

Essentially I'd imagine before the with open(mtl, 'w') as mtl_file: block something like this:

any_lines_changed = False
...
    if line.startswith("map_d "):
        lines[index] = "# " + line
        any_lines_changed = True

if not any_lines_changed:
    # Don't change file if nothing changed.
    return

@StandingPadAnimations
Copy link
Collaborator Author

Sure thing, I'll commit that on dev by the end of the day

@StandingPadAnimations
Copy link
Collaborator Author

Ight, finally pushed the requested changes (or rather a check for map_d at the start, it may be slightly inefficient, but it's a simple additional condition to the beginning if statement)

@TheDuckCow
Copy link
Member

Ight, finally pushed the requested changes (or rather a check for map_d at the start, it may be slightly inefficient, but it's a simple additional condition to the beginning if statement)

I see the branch merged, but not that last part of exiting early if no map_d's found - maybe still local on your machine?

@StandingPadAnimations
Copy link
Collaborator Author

StandingPadAnimations commented Mar 23, 2023

I pushed it directly on the dev branch
3e6c940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Known issue This is an existing MCprep behavior that, while not a bug, is known to be problematic for some users
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants