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

Use map_file in MP scenarios in mainline #4397

Closed
sevu opened this issue Sep 28, 2019 · 12 comments

Comments

@sevu
Copy link
Member

commented Sep 28, 2019

Despite map_data exists also map_file, which has been undocumented so far. Example:
map_data={multiplayer/maps/2p_Arcanclave_Citadel.map}
map_file= multiplayer/maps/2p_Arcanclave_Citadel.map

map_data parses the map at preprocessor time.
map_file does so when loading the scenario – meaning there is less to preprocess, and no need to recreate the cache if the map file has been edited.
The disadvantage however is that it doesn't display a map preview in the MP lobby.

Should we switch to map_file?
For now, I added the recommendation to the wiki, to use map_file for SP and map_data for MP.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

Not showing the map preview sounds like it'd be a problem.

@sevu

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2019

That's why I bring this up here – My thoughts so far were:
SP scenarios or rather campaigns in general (SP & MP) have never a map preview — no issue for them, we could use map_file for the campaigns.
MP – can it be changed to work with map_file ? Maybe no good idea, as it would need to read the data in some way, and do it again at scenario start.

If that's the way to go, maybe we can teach wmllint to transform the trival cases of map_data, but only if we're in a [scenario] tag. I'd like to deprecate map_data on level 1, but not remove it, as it has an legitimate use case left (having the scenario data not in an external file). In short, I want to deprecate the use of [scenario]map_data + preprocessor inclusion.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

The disadvantage however is that it doesn't display a map preview in the MP lobby.

This sounds like a bug to me that should just be fixed.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

What about fixing the bug of the preview not working for map_file, and then deprecate map_data everywhere?

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

and then deprecate map_data everywhere?

i disagree here, there are cases where one wants inline data like map_data="Gg, ..." mostly because one for example wants to have it in one file for example when uploading a testcase scenario for bugreports (there are also other usecases for that).

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

SP scenarios or rather campaigns in general (SP & MP) have never a map preview

Even in the Game Load dialog?

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

Okay, maybe don't deprecate map_data, but I'd suggest at least moving all the campaigns and (once the missing preview bug is fixed) the MP scenarios over to map_file.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

So it sounds like:

  • Don't deprecate anything, given gfgtdf's example.
  • The MP map preview not working for map_file is a bug that should be fixed.
  • Have all of the mainline campaigns and MP scenarios (once the above bug is fixed) use map_file instead of map_data.
@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

The MP map preview not working for map_file is a bug that should be fixed.

#4407

@sevu sevu added the In-progress label Oct 9, 2019
jostephd added a commit to jostephd/wesnoth that referenced this issue Oct 10, 2019
@jostephd jostephd changed the title [1.15] Considering map_file Use map_file in MP scenarios in mainline Oct 10, 2019
@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

  • The MP map preview not working for map_file is a bug that should be fixed.

Done, #4407 has been merged

  • Have all of the mainline campaigns and MP scenarios (once the above bug is fixed) use map_file instead of map_data.

This is the only remaining work here, MP scenarios only (@sevu did the SP ones in 6da85e9). Retitled/Relabeled accordingly.

@Pentarctagon

This comment has been minimized.

Copy link
Member

commented Oct 12, 2019

So everything in data/multiplayer/scenarios/?

@sevu

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2019

Feature implemented =)

@sevu sevu closed this Oct 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.