Log incomplete tileset info on load [CR]

[sparr]

When a tileset is loaded from the options menu, while in-game, this logs to the debug.log file used for other info logging. It tells which known tiles of each type are not present in the tileset.

Example output: https://gist.github.com/sparr/bada7cbc40304768480d

Unfortunately it takes about two seconds to run on my laptop after a fresh compile/load of the game, so this isn't really appropriate for normal players. I'd appreciate if someone could walk me through the steps for adding a command line parameter to enable this logging functionality. (or maybe we should have some log level options in the UI?)

Resolves #11539

[BevapDin]

Well, once performance hit are those copies here, especially the copy of the map.

Better to take that map as a const reference:

void tile_loading_report( maptype const & tiletypemap, std::string const & label, std::string const & prefix )

Edit: this function should probaly be in the cata_tiles class, in which case it doesn't need the global tilecontext.

[sparr]

Thanks for the const advice. I'll probably also move the function, especially since Jenkins says tilecontext isn't global and refuses to compile this as-is.

[BevapDin]

And another even bigger performance hit is the copy of the tile_ids, which is done for every single entry of the checked objects. There are about 2800 items, 200 vehicle parts, 200 terrain and 100 furniture types and another bunch of monster types, you make a copy of this tile_ids for every single one of those things. Make this return a const reference and store the reference (as reference, not as copy) in the functions that use it instead of calling it in each iteration.

const tile_id_map & get_tile_ids() const { return tile_ids; }

auto const & tile_ids = tile_context->get_tile_ids();

Or even better, move the logic to check tile existence into cata_tiles:

bool cata_tiles::has_tile( std::string const & id ) const
{
    return tile_ids.count( id ) > 0;
}

[sparr]

It's been a long time since I worked in C/C++ and had to do this sort of optimization by hand. Languages that start with a P have spoiled me. Thanks for all the tips.

Re the latter point, my use of tile_ids is not a check, I'm actually iterating through it, so no go on that.

[BevapDin]

Languages that start with a P have spoiled me.

Yes, they are evil. Come and join the dark, I mean the C++ side.

Re the latter point, my use of tile_ids is not a check, I'm actually iterating through it, so no go on that.

Where do you think you iterate through it? The only time you use it is to call its count function.

[BevapDin]

Use a range based loop:

for( auto const & e : tiletypemap ) {
    auto const & tile_id = prefix + e.first;
    ...

[sparr]

The devs on this project are teaching me to love C++11 every day.

[BevapDin]

Same here: a copy of the loaded tiles for every field. Do you want this code to be slow?

[BevapDin]

You might want to change MonsterGenerator::get_all_mtypes to return a const reference instead of a copy, but this copy is currently done only once, so it's not that bad.

[sparr]

I'm guessing someone else who is using that function might want a copy. I'll leave this alone, since it's only once. I wish there was a global instance of this map like furnmap and termap.

[BevapDin]

If that other places wants a copy, it will just make the copy from the returned reference instead. But as I said, that single copy here is probably no real problem.

I wish there was a global instance of this map like furnmap and termap.

There is a global instance of that map, it's encapsulated in MonsterGenerator. The global termap for example can be changed (accidentally or not) at any place by anybody which would totally screw up the game.

For example I could write termap.erase(termap.begin()) at any place and suddenly a terrain type is gone. Encapsulating those maps and proving only const access prevents this kind of error.

[BevapDin]

Item_factory::Item_factory() creates a new (temporary) Item_factory, which happens to contain no items because no items have been loaded into it. It is equivalent to Item_factory().

This is for example the same as in tile_loading_report(termap, "Terrain", std::string()); where the std::string() creates a new temporary string object which is used as an argument to the called function.

The call to get_all_itypes() happens on that empty object and will never return anything (beside the few build in types like the bionic toolset pseudo item).

You want to use the global static instance of that class:

item_controller->get_all_itypes()

Btw There are certain items that don't need a tile because the are not real and mostly only used for the crafting, e.g. "fire", "apparatus", "toolset", look at Item_factory::init_old.

Most of those maps also have null items (e.g. t_null, f_null, a null vehicle part etc.), you might want to filter those out as they are never supposed to appear - a f_null is simply no furniture at all, a t_null should never happen, each place should have a terrain etc.

[sparr]

I copied my idea for Item_factory from the existing code using MonsterGenerator. You say it should be empty, but it's not. The code as you commented on here does work.

Knowing that item_controller exists is one of my blind spots being new around here. Thanks for pointing that out!

Filtering out the fake items and nulls is a good idea. I'll leave that for a follow-up, maybe. It looks like plenty of tilesets include tiles for the nulls just in case, so I might leave that info for the tile designers who want to cover all their bases.

[BevapDin]

I copied my idea for Item_factory from the existing code using MonsterGenerator. You say it should be empty, but it's not. The code as you commented on here does work.

It compiles fine, but it won't do what you want. Example:
The ChestHole tilset does not have a tile for "biollante_bud", why is it not appearing in the list of missing tiles in your example output?

The MonsterGenerator works differently, it calls the static function MonsterGenerator::generator() which returns the single instance of that class:

static MonsterGenerator &generator()
{
    static MonsterGenerator generator;

    return generator;
}

MonsterGenerator::generator() is a normal function. Item_factory::Item_factory() creates a new temporary object.

[sparr]

@BevapDin I took most of your advice and pushed a new commit. I've made some mistake and now it compiles but won't link. I'd appreciate if you could point out what I've done wrong.

[BevapDin]

but won't link

And the error message is? Or is there no message and the linker just crashes or does it just not create the executable? Or does it just "Haha, I don't want to link this."?

I assume it complains that several versions of cata_tiles::tile_loading_report don't exist.

That's because you defined the template for those functions in cata_tiles.cpp, which is really just a template and it does not go into the compiled object. The template needs to be instantiated (which means a copy of it is created and the actual template parameters are inserted).

Your previous version did this automatically because when you called the template function in options.cpp, the compiler knew the code of the template function and could instantiate the actual functions. Now the compiler (when it complies options.cpp) only knows the function declaration (included from cata_tiles.h), but it does not known the function definitions (the actual function code which is in cata_tiles.cpp). Therefor it can not instantiate it and assumes they are instantiated somewhere else and linked in later.

There are two solutions to that problem:

• Explicitly instantiate the function in cata_tiles.cpp. Somewhere in cata_tiles.cpp (outside of the functions) write:

template
void cata_tiles::tile_loading_report< std::map<Item_tag, itype *> >(std::map<Item_tag, itype *> const & tiletypemap, std::string const & label, std::string const & prefix);
template
void cata_tiles::tile_loading_report< std::map<std::string, ter_t> >(std::map<std::string, ter_t> const & tiletypemap, std::string const & label, std::string const & prefix);
...

That is: the function declaration that has the maptype template parameter replaced with an explicit type. Do this for each maptype you'll need. This instructs the compiler to create the actual functions from the template function with the actual map types, even through the are not used when compiling cata_tiles.cpp.

• Or, the easier way: move the calls to tilecontext->tile_loading_report from options.cpp into a function in cata_tiles.cpp. The template functions are automatically instantiate like before.

For more on template instantiation, look at stackoverflow: http://stackoverflow.com/questions/8752837/undefined-reference-to-template-class-constructor

[sparr]

@BevapDin you hit the nail on the head. Thanks again for the help. Rearranged everything, pushed a new commit. It runs crazy fast now, and might be viable to include by default.