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

✨script to download, open, rename and delete matterports #158

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

Conversation

AdiWeit
Copy link

@AdiWeit AdiWeit commented Jan 12, 2025

The script associates a name with the matterport ID. Furthermore, it scrapes the name of the matterport from the matterport website (the title of the website), so it does not have to be entered manually. The script can manage your matterports by downloading, deleting, renaming and opening the one you select based on their names. Moreover, it searches for a valid URL on the website you are entering, so entering a website which embeds a matterport should also work. Finally, you can download multiple matterports successively by entering all URLs at once.
Feel free to enhance/critique the script before pulling :)

The script associates a name with the matterport id. Furthermore, it scrapes the name of the matterport from the matterport website title so it does not have to be entered manually. The script can manage your matterports by downloading, deleting, renaming and opening the one you select based on their names.
@mitchcapper
Copy link
Collaborator

mitchcapper commented Jan 12, 2025

Nice, thanks for the work! A few notes:
I dislike a single index file if it gets corrupted you are a bit screwed and if you move around downloads it won't know.
It doesn't take IDs to download like the main script (aka requiring a full url). It also doesn't work with a url format I just found (main script probably doesn't either): https://my.matterport.com/models/EGxFGTFyC9N

The way it executes the sub process has some issues. I am guessing it is somehow clearing out the ENV vars. I installed all deps into a python venv but it fails as somehow the subprocess call no longer is using the pvenv. Id say we figure out a better way to call it but per below may not be needed.

If I ask it to rename a model it should not tell me it deleted it:

input: rename 1
matterport successfully deleted
please enter the new name for the matterport: newname
renaming successful

Also if I hit control c after it asks for the new name is it just lost?

Python is not always python on every system maybe python3.
Entering a number outside the bounds of the index (ie -1 or 10) crashes.

Should be able to type the model name (or maybe even part of the name). I would first check for an exact case insensitive match but if not found take the first one that contains the entered input. To differentiate between a model name and a model ID we could probably use a length check with a regex to be safe (and if it matches an existing we just use that anyway).

I think it would be worthwhile to just integrate this into the main script. Would need to detect if its not an interactive session to avoid entering the interactive loop. Add help and adv_help commands to the cmd list.

to fix the two subprocess issues this seems to do it:
subprocess.run([sys.executable, 'matterport-dl.py', url])
subprocess.run( sys.executable, downloads[keys[int(answer) - 1]], '127.0.0.1', '8080' )

In terms of getting rid of converter.json we just need to store the alias in the download folder itself. I already have run_args.json that we could expand to store extra data. Right now any keys it doesn't know it ignores, so we can store additional data and have a way to access unknown args. LoadFromFile right now only stores things that are known by if arg.arg.name in config: but if we store unknown into a dictionary like otherSettings we could add an accessor for those.

Given the naming though we actually don't need to do this, there is the "alias" value so we can reuse that to store the name and for the menu.

Delete needs a confirmation and id like to see "Please type model ID or alias X: to confirm deleting". Some models are no longer on matterport.dl and would hate to have someone type delete 3 and lose an important model when they meant to delete 2.

You don't need to make these changes but I probably would go down this path before integrating it into the main repo.

I am thinking we may want to add the IP and port to CLI args as well (still allow current format but also allow say --ip and --port) this would allow these to be saved in the model config so we are not just hard coding a 127.0.0.1 8080 launch.

Open to thoughts but think it is certainly a nice usability improvement!

@AdiWeit
Copy link
Author

AdiWeit commented Jan 12, 2025

Thank you a lot for your detailed feedback! Honestly I don't really have the time to work on it right now, but I will look into it in about a month 👍

@mitchcapper
Copy link
Collaborator

Thank you a lot for your detailed feedback! Honestly I don't really have the time to work on it right now, but I will look into it in about a month 👍

I appreciate you taking the time to get the PR together as it is, and as I said if you don't get a chance I will probably do so at some point before merging. Just update the ticket if you do start working on it so we don't accidentally double up the work.

I think it will certainly be a better user experience (and we should be able to keep all the existing CLI functionality for those who don't want to use it).

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