Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign uptilesets: add support for looks_like field #24747
Conversation
mlangsdorf
added some commits
Aug 8, 2018
This comment has been minimized.
This comment has been minimized.
mlangsdorf
force-pushed the
mlangsdorf:looks_like_tile
branch
to
cde1efc
Aug 9, 2018
This comment has been minimized.
This comment has been minimized.
|
Will add doc/ changes tomorrow. Sorry I forgot. |
ZhilkinSerg
added
(P2 - High)
[JSON]
Mods
SDL: Tiles / Sound
[C++]
labels
Aug 9, 2018
This comment has been minimized.
This comment has been minimized.
|
It Seems good, but overlay_XXXX tilles is not taken into account. Support them is hard to implement? |
This comment has been minimized.
This comment has been minimized.
|
As far as I can tell, overlay tiles are handled in draw_entity_with_overlays(), which pulls the overlay string ids from pl.get_overlay_ids() and then looks for overlay_(fe)male_$OVERLAY or overlay_$OVERLAY via find_tile_type(). If one of those two matches, draw_from_string_id() is called, but it will necessarily return a tile because draw_from_string_id doesn't get called unless there's a tile to return. overlays aren't terrain, furniture, monsters, vehicleparts, or items. They don't have an json object associated with them that can have a looks_like value. I don't think looks_like overlays would necessarily even be a good idea. But maybe I'm wrong. I think I can code up a find_mutation_looks_like and find_effects_looks_like fairly straightforwardly, and the existing find_item_looks_like could be altered to take an optional prefix string and perform the search with the prefix string. The biggest problem is the number of the potential searches: overlay_male_ultimate_winter_survivor_coat might have to search for overlay_ultimate_winter_survivor_coat, overlay_male_winter_survivor_coat, overlay_winter_survivor_coat, overlay_male_duster_survivor, overlay_duster_survivor, overlay_male_duster_leather, overlay_duster_leather, overlay_male_duster, overlay_duster, overlay_male_trenchcoat, overlay_trenchcoat, overlay_male_coat_winter, and overlay_coat_winter to satisfy the looks_like series of though I guess that's only 6 loops through and 24 searches (because you have to search for seasonal variations each time), not too bad. I suspect the find() operator is O(1), so doing it 40 times (in the absolute worst case scenario) per tile shouldn't be too bad, and I don't expect most tiles to need the full 10 iterations to either find a matching tile or run out of looks_like. Might take me a couple of days. Thinking about it, I'd rather open a second PR for overlays after this gets merged, because I'd like this to get extensive reality testing before expanding it, if that's okay by you. |
This comment has been minimized.
This comment has been minimized.
|
Addition: |
mlangsdorf
referenced this pull request
Aug 10, 2018
Open
tileset looks_like: further enhancements #24758
This comment has been minimized.
This comment has been minimized.
|
Artifact tagging added to the todo list but not for this PR. |
This comment has been minimized.
This comment has been minimized.
|
minimal documentation added. Off to go revise JSON_INFO. |
This comment has been minimized.
This comment has been minimized.
|
After this PR is merged, you can also update info on json API changes in #19376. |
ZhilkinSerg
self-assigned this
Aug 10, 2018
ZhilkinSerg
merged commit 1280b85
into
CleverRaven:master
Aug 10, 2018
ZhilkinSerg
removed their assignment
Aug 10, 2018
mlangsdorf
deleted the
mlangsdorf:looks_like_tile
branch
Aug 10, 2018
mlangsdorf
referenced this pull request
Aug 11, 2018
Merged
tilesets and looks_like: add some looks_like to cover ChestHole gaps #24768
codemime
reviewed
Aug 12, 2018
| const std::string &subcategory, tripoint pos, | ||
| int subtile, int rota, lit_level ll, | ||
| bool apply_night_vision_goggles, int &height_3d ) | ||
| const tile_type *cata_tiles::find_furniture_looks_like( std::string id ) |
This comment has been minimized.
This comment has been minimized.
codemime
Aug 12, 2018
Member
Immutable strings should be passed by constant references: const std::string& id.
codemime
reviewed
Aug 12, 2018
| { | ||
| std::string looks_like = id; | ||
| int cnt = 0; | ||
| while( !looks_like.empty() && cnt < 10 ) { |
This comment has been minimized.
This comment has been minimized.
codemime
Aug 12, 2018
Member
You could've stored already visited items using a set or something, thus preventing infinite loops. The counter based approach raises questions like "why 10"? Checking for inconsistencies beforehand would be even better.
codemime
reviewed
Aug 12, 2018
| std::string looks_like = id; | ||
| int cnt = 0; | ||
| while( !looks_like.empty() && cnt < 10 ) { | ||
| const tile_type *lltt = find_tile_with_season( looks_like ); |
This comment has been minimized.
This comment has been minimized.
codemime
Aug 12, 2018
Member
find_tile_with_season() is invoked twice per draw_from_id_string(). The first invocation is here.
This comment has been minimized.
This comment has been minimized.
mlangsdorf
Aug 12, 2018
Author
Contributor
I think the issue you're pointing out is addressed in #24778
This comment has been minimized.
This comment has been minimized.
codemime
Aug 12, 2018
Member
I meant something else:
const tile_type *tt = find_tile_with_season( id ); // <- 1st find_tile_with_season(id).
if( !tt ) {
...
if( category == C_FURNITURE ) {
tt = find_furniture_looks_like( id, effective_id ); // <- 2nd find_tile_with_season(id) with the same id inside the functionMap lookup times are noticeable in tiles, so this may slightly impact the overall performance.
codemime
reviewed
Aug 12, 2018
| return nullptr; | ||
| } | ||
|
|
||
| const tile_type *cata_tiles::find_terrain_looks_like( std::string id ) |
This comment has been minimized.
This comment has been minimized.
codemime
Aug 12, 2018
•
Member
These functions look very alike. They introduce unnecessary duplication which can be avoided with something like:
const tile_type *cata_tiles::find_tile_looks_like( const std::string& id, TILE_CATEGORY category )
{
switch( category )
{
...
case C_FURNITURE: {
const furn_str_id fid( id );
return fid ? fid->looks_like : nullptr;
}
...
}
return nullptr;
}
This comment has been minimized.
This comment has been minimized.
mlangsdorf
Aug 12, 2018
Author
Contributor
Yeah, I realized that after I submitted it. Refactoring it is on the todo list.
This comment has been minimized.
This comment has been minimized.
|
The very idea of using
|
This comment has been minimized.
This comment has been minimized.
|
Completely disagree on the second point. Most of the time it doesn't restate copy-from at all. None of the hd vehicle door/hatches/trunk doorss are copy-froms of the non-hd versions. As far as the first point, I don't see any other place to put this information than in the item JSON. Any other location is going to be inherently fragile. |
This comment has been minimized.
This comment has been minimized.
Good point. This looks like an issue to me. You've outlined it and offered a workaround which would work for tiles, but only if people are willing to use it.
JSON is perfectly fine for the purpose. I just meant that if you store the ancestor's id somewhere (the item |
This comment has been minimized.
This comment has been minimized.
|
Using of |
This comment has been minimized.
This comment has been minimized.
This PR relies solely on |
This comment has been minimized.
This comment has been minimized.
|
See #24768 |
lispcoc
reviewed
Aug 13, 2018
| return false; | ||
| } | ||
|
|
||
| const tile_type *tt = find_tile_with_season( id ); |
This comment has been minimized.
This comment has been minimized.
lispcoc
Aug 13, 2018
Contributor
I think id is not updated to seasonal_id. May this cause break seasonal tile? #24819


mlangsdorf commentedAug 9, 2018
This adds support for a looks_like field in most things that have tileset images.
If the tileset loader fails to load a tile for a terrain, furniture, monster, vehicle part, or item, it will check to
see if the object has a looks_like field. If it does, it will attempt to load that tile. If that fails, it will check
if the looks_like object has a field, repeating the process up to 10 times.
the looks_like defaults to the value of copy-from if the object has a copy-from field, or it can be set
explicitly.
Fixes #19932