Skip to content

map meta lookup: only check for map in packs starting with “map-” prefix, fix #285#315

Merged
illwieckz merged 1 commit intoDaemonEngine:masterfrom
illwieckz:mapprefix
Apr 16, 2020
Merged

map meta lookup: only check for map in packs starting with “map-” prefix, fix #285#315
illwieckz merged 1 commit intoDaemonEngine:masterfrom
illwieckz:mapprefix

Conversation

@illwieckz
Copy link
Copy Markdown
Member

Map meta lookup: only check for map in packs starting with map- prefix, fix #285.

  • don't look for map in packs whose name is too small to have a map-prefix
  • don't look for map in packs not starting with map- prefix
  • don't look for map in packs with empty string after map- prefix

Note: looking for stuff in meta/ looks to be game-specific.

@illwieckz illwieckz force-pushed the mapprefix branch 3 times, most recently from 4de32c0 to 722f86c Compare April 12, 2020 05:07
@illwieckz illwieckz force-pushed the mapprefix branch 4 times, most recently from a133457 to c4ec828 Compare April 13, 2020 09:28
…fix, fix DaemonEngine#285

- don't look for map in packs whose name is too small to have a map-prefix
- don't look for map in packs not starting with “map-” prefix
- don't look for map in packs with empty string after “map-” prefix

- fix DaemonEngine#285 bug
- may speedup map listing

Note: looking for stuff in ”meta/” looks to be game-specific.
{
const std::string pak_map_prefix("map-");

for (const auto& pak: FS::GetAvailableMapPaks()) {
Copy link
Copy Markdown
Member

@slipher slipher Apr 13, 2020

Choose a reason for hiding this comment

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

GetAvailableMapPaks is already supposed to filter for paks that start with map-, but the filtering is disabled when fs_legacypaks is on. So either that function should be changed to always return only paks starting with map-, or the behavior here should do something different when the map does not start with map-.

Copy link
Copy Markdown
Member Author

@illwieckz illwieckz Apr 14, 2020

Choose a reason for hiding this comment

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

This is to load meta information to build the create server ui, which is a game thing.

We may have to keep the test for the map- prefix in both.

This one is to filter maintained maps that were packaged the right way, we are not expecting legacy paks to have meta information. So, since GetAvailableMapPaks may list all map packs even the legacy ones, we have to filter in FS_LoadAllMapMetadata

We can either drop the map- prefix test in GetAvailableMapPaks, but we can keep it for the sole purpose of speeding-up things for when support for legacy packs is disabled.

The map- prefix for maps is part of DPK format definition, so this is OK to keep the test for it in engine wen looking for map packs when support for legacy packs is disabled.

That FS_LoadAllMapMetadata function really looks like a game-specific feature, especially Unvanquished specific feature. We may imagine other games doing things differently than the meta/ thing (even if we strongly recommend to do so, as best practices and best support). Or we have to decide the meta thing is part of the DPK format (I'm not sure about it, if we standardize it, it would be better to make it an optional extension).

At this point it's fairly legit to think a game may extends FS_LoadAllMapMetadata, extending the existing meta stuff with checks for other games (like legacy .arena files). It's maybe not up to the engine to support all existing meta formats.

Also, we can imagine a game wanting to support more formats would rely on this unmodified FS_LoadAllMapMetadata but add another function to complete the list with other formats. In this case we may remove the map- prefix and make sure this function is never used for other formats.

@slipher
Copy link
Copy Markdown
Member

slipher commented Apr 15, 2020

LGTM

1 similar comment
@dimhotepus
Copy link
Copy Markdown
Contributor

LGTM

@illwieckz illwieckz merged commit 24567df into DaemonEngine:master Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled exception (St12out_of_range) when displaying map list window if dpk with small name is in path

3 participants