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

Editor: --edit-level now also takes an absolute path #1835

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tobbi
Copy link
Member

@tobbi tobbi commented Oct 5, 2021

Load levels and worldmaps from the filesystem instead of using a PHYSFS local reference.

Fixes #1819

Please test this!

@tobbi tobbi changed the title Editor load filesystem Editor: --edit-level now also takes an absolute path Oct 5, 2021
@Semphriss
Copy link
Member

Tested; works as described.

I'd recommend it takes only a relative path as argument, to avoid name clashes - if someone wants to test a few levels in a folder and a path for a level happens to clash with one in the data folder, the behavior might seem weird for someone who doesn't read the docs regularly. Giving priority to relative paths would be better, but it could still grab paths that don't exist.

Was there a particular reason to make it take a path starting from the data directory in the first place? As a CLI user, I find it more intuitive to enter either an absolute path or a relative path from the current directory. Also, paths from the data directory don't support autocomplete.

@tobbi
Copy link
Member Author

tobbi commented Oct 6, 2021

Still need to fix the shadowing issue.

@tobbi tobbi marked this pull request as draft October 6, 2021 06:48
@tobbi
Copy link
Member Author

tobbi commented Oct 16, 2021

@Semphriss Can you please specify in what order we should check paths?

My suggestion would be:
a) Absolute paths in the user's filesystem
b) Relative paths in the user's filesystem
c) PhysFS mounts

You concur?

@Semphriss
Copy link
Member

@Semphriss Can you please specify in what order we should check paths?

My suggestion would be: a) Absolute paths in the user's filesystem b) Relative paths in the user's filesystem c) PhysFS mounts

You concur?

I'd propose only absolute/relative from CWD; testing multiple paths might be confusing for users when paths clash. If we really need to have both options available, it would be wise to let the user specify their preference with a CLI flag, to avoid making certain level files unplayable under certain circumstances.

@tobbi tobbi force-pushed the editor-load-filesystem branch from 38a5bc5 to e0f5200 Compare March 7, 2022 20:11
@tobbi tobbi marked this pull request as ready for review March 7, 2022 20:14
@@ -115,6 +116,7 @@ CommandLineArguments::print_help(const char* arg0) const
<< _("Game Options:") << "\n"
<< _(" --edit-level Open given level in editor") << "\n"
<< _(" --resave Loads given level and saves it") << "\n"
<< _(" --from-datadir Indicates that the LEVELFILE path is relative to the data dir. Only used if --edit-level is specified") << "\n"
Copy link
Member

@Semphriss Semphriss Mar 9, 2022

Choose a reason for hiding this comment

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

IMO just --from-datadir isn't clear enough, it leaves confusion as to whether only the following path is from the datadir or if all of them are. To fix this, I'd suggest --paths-from-datadir (It also makes it clear in an instant that this is about paths).

I also recommend changing "Indicates that the LEVELFILE path is relative to..." to "Indicates that the paths are relative to..." since all paths on the CLI should be affected by this.

This was referenced Mar 17, 2022
@weluvgoatz weluvgoatz added the status:needs-discussion Team member and developers need to discuss of decisions label Sep 12, 2022
@mrkubax10 mrkubax10 added involves:editor involves:functionality status:in-progress Progress has been done, but more is intended be done type:feature category:code and removed status:needs-discussion Team member and developers need to discuss of decisions labels Aug 1, 2023
@tobbi tobbi force-pushed the editor-load-filesystem branch from 4839715 to 7d7268d Compare August 30, 2023 12:13
@mrkubax10
Copy link
Member

Better late than never :^)

@mrkubax10 mrkubax10 added the status:needs-work In progress, but no one is currently working on it (New volunteers welcome) label Oct 7, 2023
@tobbi tobbi force-pushed the editor-load-filesystem branch from 794bfba to 7f6ec99 Compare March 29, 2024 16:53
@tobbi
Copy link
Member Author

tobbi commented Mar 29, 2024

What paths should actually be affected by this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code involves:editor involves:functionality status:in-progress Progress has been done, but more is intended be done status:needs-work In progress, but no one is currently working on it (New volunteers welcome) type:feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI option --edit-level takes path relative to data directory, inconsistent with normal play invocation
4 participants