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

More portable paths for filenames in campaigns and add-ons, like [binary_path] #4633

Closed
stevecotton opened this issue Dec 11, 2019 · 15 comments
Closed
Labels

Comments

@stevecotton
Copy link
Contributor

@stevecotton stevecotton commented Dec 11, 2019

The directory name used for a campaign or add-on ends up hardcoded in multiple places in it. For example, once in each scenario:

01_Rooting_Out_a_Mage.cfg:    map_file=campaigns/Two_Brothers/maps/01_Rooting_Out_a_Mage.map
02_The_Chase.cfg:    map_file=campaigns/Two_Brothers/maps/02_The_Chase.map
etc

Every one of those also hardcodes whether it's a built-in campaign or a UMC campaign, which means every AOI scenario will need to change if it moves to UMC.

For images, there's already [binary_path], for other files there should be something that means "the path for this add-on".

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 11, 2019

#2180 would be one method, by accepting ../ in relative paths, but this means checking code that allows directory traversal. If we just want to access files inside the same add-on, maybe it can be done by the add-on itself?

For example, in _main.cfg use #define to store the value of {CURRENT_DIRECTORY}, and then use the new def to access other files.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Dec 11, 2019

for the simple map_file case we could also.just extand binary_path to.map files (make wesnoth search map_file s in all [binary_patg]/maps

For example, in _main.cfg use #define to store the value of {CURRENT_DIRECTORY}, and then use the new def to access other files.

This doesn't work directly like that because Marcos inside ma Ros are afaik substituted when the outer.macro.is used and not when they are defined

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Dec 13, 2019

I definitely prefer #2180 for this. If someone else wants to implement it in a simpler way, be my guest; otherwise I'll eventually get back to it.

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 13, 2019
…esnoth#4633)

WIP: Only contains the AToTB and HttT changes so far

Allows both of these to have the same effect, thus allowing a load of copies
of the campaign/add-on's name to be omitted:

* map_file=campaigns/Heir_To_The_Throne/maps/01_The_Elves_Besieged.map
* map_file=01_The_Elves_Besieged.map

This makes [binary_path] a misnomer, as it now also handles a text-based type
of file, however that's going to be the correct path for campaigns or add-ons
that use the standard layout with images/, maps/, scenarios/, etc.

This meaning that an add-ons' name can be changed, or the campaign moved
between UMC and mainline, without needing to change every scenario's .cfg file.
@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 13, 2019

for the simple map_file case we could also just extend binary_path to map files (make wesnoth search map_files in all [binary_path]/maps

That's actually really easy to implement. It makes [binary_path] a misnomer, as it now also handles a text-based type of file, however that's going to be the correct path for campaigns or add-ons that use the standard layout with images/, maps/, scenarios/, etc.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 13, 2019

Delfador's Memoirs goes for a simpler option, just making a macro for the hardcoded path:

#define MEMOIRS_MAP NAME
map_file=campaigns/Delfadors_Memoirs/maps/{NAME}
#enddef

I still think the functionality should be added to map_file=, but DM's method is simple and works with stable versions of Wesnoth.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Dec 14, 2019

That's actually really easy to implement.

Yeah, it's really easy, indeed. I still prefer the .. method though.

It makes [binary_path] a misnomer, as it now also handles a text-based type of file

Actually it's also already used for po files, if I recall correctly. (Maybe mo files too, I forget.)

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Dec 14, 2019

Yeah, it's really easy, indeed. I still prefer the .. method though.

This is about map_file and not macro inclusion, the code that uses map_file does not know the location of the wml file that defined the scenario.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

@CelticMinstrel CelticMinstrel commented Dec 14, 2019

Ah, you do have a point there. I guess it's reasonable to implement both things then.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Dec 16, 2019

It'd be nice if those new parts would also be supported by wml that's (replace map) etc that take a map_file parameter

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 16, 2019

I'm curious about the regex check in replace_map - as it's already limited to files accessible via get_wml_location, what is this check meant to prevent?

//Do a regex check for the file format to prevent sending arbitrary files to other clients.
//Note: this allows only the new format.
static const std::string s_simple_terrain = R"""([A-Za-z\\|/]{1,4})""";
static const std::string s_terrain = s_simple_terrain + R"""((\^)""" + s_simple_terrain + ")?";
static const std::string s_sep = "(, |\\n)";
static const std::string s_prefix = R"""((\d+ )?)""";
static const std::string s_all = "(" + s_prefix + s_terrain + s_sep + ")+";
static const boost::regex r_all(s_all);
const std::string& mapfile = filesystem::get_wml_location(filename_);
std::string res = "";
if(filesystem::file_exists(mapfile)) {
res = filesystem::read_file(mapfile);
}
config retv;
if(boost::regex_match(res, r_all))
{
retv["map_data"] = res;
}
return retv;

@gfgtdf

This comment has been minimized.

Copy link
Contributor

@gfgtdf gfgtdf commented Dec 16, 2019

I'm curious about the regex check in replace_map

I'm quite sure that I wrote that code but I don't remember why, I was probably just extra cautious. In particular with the existence of wesnoth.read_file nowdays there is little point in these checks.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 17, 2019

@sevu your 8080271 (without any changes from this issue) seems to be broken - Endless Night crashes Wesnoth once enough repetitions have happened to cause the [replace_map] to fire.

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 17, 2019
Fixes 8080271's Eternal Night. The northern
parts of both 10 and 10a are the same, but 10a is extended southwards; this
caused some confusion thinking that the map-load hadn't changed anything.

The regexp code is removed, as gtgfdf comments in wesnoth#4633: "I'm quite sure that I
wrote that code but I don't remember why, I was probably just extra cautious.
In particular with the existence of wesnoth.read_file nowdays there is little
point in these checks."

Removing the regexp code allowed merging with the codepath for handling
[scenario]map_file=, which will allow the main issue of wesnoth#4633 (searching other
folders for maps) to automatically affect [replace_map] too.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 17, 2019
…esnoth#4633)

WIP: Only contains the AToTB, HttT and Tutorial changes so far

Allows both of these to have the same effect:

* map_file=campaigns/Heir_To_The_Throne/maps/01_The_Elves_Besieged.map
* map_file=01_The_Elves_Besieged.map

This allows a lot of copies of the campaign/add-on's name to be omitted. It's
no longer necessary to change every scenario's .cfg file to rename an add-on,
or to move a campaign between UMC and mainline.

This makes [binary_path] a misnomer, as it now also handles a text-based type
of file, however that's going to be the correct path for campaigns or add-ons
that use the standard layout with images/, maps/, scenarios/, etc.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 17, 2019
Fixes 8080271's Eternal Night. The northern
parts of both 10 and 10a are the same, but 10a is extended southwards; this
caused some confusion thinking that the map-load hadn't changed anything.

The regexp code is removed, as gfgtdf comments in wesnoth#4633: "I'm quite sure that I
wrote that code but I don't remember why, I was probably just extra cautious.
In particular with the existence of wesnoth.read_file nowdays there is little
point in these checks."

Removing the regexp code allowed merging with the codepath for handling
[scenario]map_file=, which will allow the main issue of wesnoth#4633 (searching other
folders for maps) to automatically affect [replace_map] too.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 17, 2019
…esnoth#4633)

WIP: Only contains the AToTB, HttT and Tutorial changes so far

Allows both of these to have the same effect:

* map_file=campaigns/Heir_To_The_Throne/maps/01_The_Elves_Besieged.map
* map_file=01_The_Elves_Besieged.map

This allows a lot of copies of the campaign/add-on's name to be omitted. It's
no longer necessary to change every scenario's .cfg file to rename an add-on,
or to move a campaign between UMC and mainline.

This makes [binary_path] a misnomer, as it now also handles a text-based type
of file, however that's going to be the correct path for campaigns or add-ons
that use the standard layout with images/, maps/, scenarios/, etc.
stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 17, 2019
Fixes 8080271's Eternal Night. The northern
parts of both 10 and 10a are the same, but 10a is extended southwards; this
caused some confusion thinking that the map-load hadn't changed anything.

The regexp code is removed, as gfgtdf comments in wesnoth#4633: "I'm quite sure that I
wrote that code but I don't remember why, I was probably just extra cautious.
In particular with the existence of wesnoth.read_file nowdays there is little
point in these checks."

Removing the regexp code allowed merging with the codepath for handling
[scenario]map_file=, which will allow the main issue of wesnoth#4633 (searching other
folders for maps) to automatically affect [replace_map] too.
stevecotton added a commit that referenced this issue Dec 17, 2019
Fixes 8080271's Eternal Night. The northern
parts of both 10 and 10a are the same, but 10a is extended southwards; this
caused some confusion thinking that the map-load hadn't changed anything.

The regexp code is removed, as gfgtdf comments in #4633: "I'm quite sure that I
wrote that code but I don't remember why, I was probably just extra cautious.
In particular with the existence of wesnoth.read_file nowdays there is little
point in these checks."

Removing the regexp code allowed merging with the codepath for handling
[scenario]map_file=, which will allow the main issue of #4633 (searching other
folders for maps) to automatically affect [replace_map] too.
@sevu

This comment has been minimized.

Copy link
Member

@sevu sevu commented Dec 18, 2019

@stevecotton thanks. Did the merged PR fix it?

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 18, 2019

@sevu yes :)

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Dec 20, 2019
…oth#4633)

For both [scenario]map_file= and [replace_map]map_file=, this allows both of
these to have the same effect:

* map_file=campaigns/Heir_To_The_Throne/maps/01_The_Elves_Besieged.map
* map_file=01_The_Elves_Besieged.map

This allows a lot of copies of the campaign/add-on's name to be omitted. Thus
it's no longer necessary to change every scenario's .cfg file to rename an
add-on, or to move a campaign between UMC and mainline.

This makes [binary_path] a misnomer, as it now also handles a text-based type
of file, however that's going to be the correct path for campaigns or add-ons
that use the standard layout with images/, maps/, scenarios/, etc.

This commit has the change itself, in filesystem_common.cpp, and the updates
for most of the campaigns. DM, LoW, UtBS and WoV are omitted from this, as they
all use a macro to do the same effect:

 #define MEMOIRS_MAP NAME
-    map_file=campaigns/Delfadors_Memoirs/maps/{NAME}
+    map_file={NAME}
 #enddef
stevecotton added a commit that referenced this issue Dec 26, 2019
For both [scenario]map_file= and [replace_map]map_file=, this allows both of
these to have the same effect:

* map_file=campaigns/Heir_To_The_Throne/maps/01_The_Elves_Besieged.map
* map_file=01_The_Elves_Besieged.map

This allows a lot of copies of the campaign/add-on's name to be omitted. Thus
it's no longer necessary to change every scenario's .cfg file to rename an
add-on, or to move a campaign between UMC and mainline.

This makes [binary_path] a misnomer, as it now also handles a text-based type
of file, however that's going to be the correct path for campaigns or add-ons
that use the standard layout with images/, maps/, scenarios/, etc.

This commit has the change itself, in filesystem_common.cpp, and the updates
for most of the campaigns. DM, LoW, UtBS and WoV are omitted from this, as they
all use a macro to do the same effect:

 #define MEMOIRS_MAP NAME
-    map_file=campaigns/Delfadors_Memoirs/maps/{NAME}
+    map_file={NAME}
 #enddef
@stevecotton

This comment has been minimized.

Copy link
Contributor Author

@stevecotton stevecotton commented Dec 26, 2019

Implemented in #4654

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.