Fix map memory edge case #37460
Fix map memory edge case #37460
Conversation
esotericist
added
<Bugfix>
src/cata_tiles.cpp
Outdated
| g->u.memorize_tile( abs_pos, tname, subtile, rotation ); | ||
| } else { | ||
| const memorized_terrain_tile &cached_tile = g->u.get_memorized_tile( abs_pos ); | ||
| if( cached_tile.subtile != subtile && cached_tile.tile == tname ) { |
There was a problem hiding this comment.
rotation also affects appearance of tiles, so it should also be compared. Anyway, I think you can just memorize the tile without comparing it to the old value.
Also I'm not sure if it fully fixes #36609. Could you test this case:
- Move to a place with low light-level and straight walls, so you can only see a 3x3 area around the character.
- Move near the wall so you can see three wall tiles, which are now memorized.
- Move away from the wall so you can't see it, move 3 steps along the wall's direction, and then move near the wall again, so you see another three wall tiles.
- Does the six wall tiles look like this:
======(what we want) or==><==(what we had)?
There was a problem hiding this comment.
Rotation does change the appearance of the tile, but I don't think rotation should be changed without resetting the cache at that index, whereas revealing more information about the map does change the subtile without telling the cache.
And I believe the tile needs to be compared to the old one because (as far as I can tell) only the top level "thing" is stored in map memory. Furniture, for example, could be overwritten by terrain, as only one can be remembered. So if something has been seen there, it can't be overwritten unless I know it's terrain.
An improvement I might actually be able to do is to just check if it's terrain and overwrite it.
Another option is to un-see the tiles being drawn before re-seeing them (so they are re-memorized), but I feel like this would remove the purpose of the map memory seen cache, if I'm thinking correctly.
And I had noticed that situation in the test would happen, but I would consider that a different bug. You can also fix the memory by just sweeping over the disconnections, and it'll fix it. I can't think of a convenient way to fix that bug.
src/cata_tiles.cpp
Outdated
| @@ -2179,7 +2182,8 @@ bool cata_tiles::draw_terrain( const tripoint &p, const lit_level ll, int &heigh | |||
| // do something to get other terrain orientation values | |||
| } | |||
| const std::string &tname = t.id().str(); | |||
| if( g->m.check_seen_cache( p ) ) { | |||
| // re-memorize seen terrain in case new connections have been formed | |||
There was a problem hiding this comment.
I'm very concerned that this defeats the caching entirely, and possibly pathologically, which is incidentally why I haven't taken a stab at it myself, I can't see a way to fix it without breaking the cache.
The whole point of the cache is to not call memorize_tile(), but has_terrain_memory_at() is nearly as expensive, and is very frequently going to return true, meaning we call memorize_tile() anyway.
I might have a fix though, we can bypass the cache when if( t.obj().connects( connect_group ) ) { a few lines up fires. that pessimizes for any connecting tiles, but those should be relatively rare, and gets us re-memorization.
However, that does mean that the final memorized tile is going to be the "last one seen", so it's still not going to be always the most correct, that would require us to have an ordering of which tiles are preferred somehow so it doesn't "downgrade".
Summary
SUMMARY: NonePurpose of change
Fixes #37318
Fixes #36609
Describe the solution
Tiles were previously only being solidified into map memory if they were off screen; tiles on screen were being memorized every time they were drawn until they were memorized off screen.
Tiles also couldn't be re-memorized if something changed. So if the player learned that a tile was connected to another tile, changing its appearance, it wouldn't change map memory if the tile was already solidified into memory.
If the tile has already been seen, it will check the cache to see if it has a different subtile so it can fix it.
Describe alternatives you've considered
I considered trying to remove the string comparison, but it should rarely get executed, and it's not worth the memory/hassle.
I also don't believe this is an issue for anything other than connecting terrain.
Testing
Walk around, memory should work.
Memory with stairs and z-levels off should work, it broke sometimes before.
Memory should work with teleporting, it previously wouldn't allow memorizing in the area you tp'd to.
Use a tileset that shows proper tile connections.
Load a tile that has an unseen connection to it, make sure it's on screen when you load it.
Zoom in or shift the camera so that it's in view of the character, but it's not on screen.
Reset the camera back to normal.
Load the adjacent tile that was previously unseen, and leave the sight of both tiles.
Previously, one of the tiles wouldn't have shown a connection in map memory, it should now.
Additional context
I'm not 100% confident this fixes #37318 because I found it hard to reproduce, but I do believe it's the same problem as the tile connection issue.
Tell me if it's spaghetti.