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

support for Linux-native studiomdl #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

JJL772
Copy link

@JJL772 JJL772 commented Nov 30, 2022

Some games like Momentum Mod and Portal 2: Community Edition ship a Linux-native version of studiomdl. This PR adds support for Linux-native studiomdl if it exists.

Summary of changes:

  • Check for studiomdl instead of only studiomdl.exe
  • Rework some of that bin directory finding code
  • Don't re-resolve bin directory if game directory didn't actually change (this was mainly just an annoyance for me when dealing with non-standard modelsrc,etc paths)

I wasn't able to test this on Windows yet, just opening for preliminary review I guess

@bonjorno7
Copy link
Owner

Oh thanks!
I'll review it tomorrow and test on windows.

Copy link
Owner

@bonjorno7 bonjorno7 left a comment

Choose a reason for hiding this comment

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

Sorry it took so long, I forgor.
I hope it puts the messages from top to bottom instead of chronologically.

Comment on lines 37 to 40
studiomdl = self.bin.joinpath('studiomdl.exe')
studiomdl = game_utils.get_studiomdl_path(game)
quickmdl = self.bin.joinpath('quickmdl.exe')
self.studiomdl = quickmdl if quickmdl.is_file() else studiomdl
Copy link
Owner

Choose a reason for hiding this comment

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

Let's move the QuickMDL thing into get_studiomdl_path too.

Copy link
Author

Choose a reason for hiding this comment

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

Does QuickMDL support Linux too?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I don't know actually.. I assume it works through Wine but don't know if there's a native binary.

@@ -7,6 +7,7 @@
from pathlib import Path
from traceback import print_exc
from ... utils import common
from ... utils import game as game_utils
Copy link
Owner

Choose a reason for hiding this comment

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

I don't love importing as; would it be possible to import utils itself and then use utils.common and utils.game everywhere in this file?
Don't pay it any mind if you think it's a bad idea.

Copy link
Author

Choose a reason for hiding this comment

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

Added this as a lazy workaround for a parameter with the name game :trollface:

will change though

addon/types/model_export/model.py Show resolved Hide resolved

import os
Copy link
Owner

Choose a reason for hiding this comment

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

Move this import above the others please.

Comment on lines 6 to 11
self['game'] = resolve(self.game)
g = resolve(self.game)
# Don't bother re-resolving paths if we haven't changed our game path
if g == self['game']:
return

self['game'] = g
Copy link
Owner

Choose a reason for hiding this comment

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

As far as I know, Blender does not call an update function if the value did not change, so this check shouldn't be necessary.

Comment on lines 42 to 64
studiomdl = Path(game.bin).joinpath('studiomdl.exe')
return gameinfo.is_file() and studiomdl.is_file()
return gameinfo.is_file() and get_studiomdl_path(game).is_file()

def get_studiomdl_path(game):
if os.name == 'posix' and (game.bin.endswith('linux32') or game.bin.endswith('linux64')):
p = Path(game.bin).joinpath('studiomdl')
if p.is_file():
return p
return Path(game.bin).joinpath('studiomdl.exe')

Copy link
Owner

Choose a reason for hiding this comment

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

Same point about variable names here.
Sorry to have so many nitpicks, I really do appreciate your contribution.

Copy link
Owner

Choose a reason for hiding this comment

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

This message is supposed to show up below the next one.
The aformentioned point about variable names is that I'd like longer ones.

Comment on lines 12 to 30
if not bin.joinpath('studiomdl.exe').is_file():
for path in bin.iterdir():
if path.is_dir() and path.joinpath('studiomdl.exe').is_file():
bin = path
break
# If we're not on the old-style pattern bin/<something> layout, check for platform subdirs
actualbin = None
if not bin.joinpath('studiomdl.exe').is_file() and not bin.joinpath('studiomdl').is_file():
def check_subdir(subdirs, smdl):
for s in subdirs:
p = bin.joinpath(s)
if p.is_dir() and p.joinpath(smdl).is_file():
return p
return None

# For linux, prefer the native binaries (if possible)
if os.name == 'posix':
actualbin = check_subdir(['linux32', 'linux64'], 'studiomdl')
# Resolve windows paths
if actualbin is None:
actualbin = check_subdir(['win32', 'win64'], 'studiomdl.exe')

if actualbin is not None:
bin = actualbin
Copy link
Owner

Choose a reason for hiding this comment

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

Kind of a nitpick but I'm not a fan of these short variable names, I think renaming s to subdir and p to path would make it easier to read.
I'll probably refactor this function a little anyway after merging though.

@bonjorno7
Copy link
Owner

@JJL772 The force push makes it kind of confusing to review but I'll try.

@JJL772
Copy link
Author

JJL772 commented Sep 20, 2023

@bonjorno7 Sorry for the force push! Totally forgot about this PR until now- is there anything else you want me to do here?

@bonjorno7
Copy link
Owner

It's been 10 months and I've completely forgotten what this is about lol

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.

2 participants