Do not rebuild map cache when it is not necessary #29747
Conversation
ZhilkinSerg
added
Code: Performance
ZhilkinSerg
force-pushed
the
z-levels-map-cache-invalidation
branch
2 times, most recently
from
3eb2884 to
b75c702
Compare
src/game.h
Outdated
| @@ -572,6 +572,10 @@ class game | |||
| int assign_npc_id(); | |||
| Creature *is_hostile_nearby(); | |||
| Creature *is_hostile_very_close(); | |||
| bool is_map_cache_valid = false; | |||
There was a problem hiding this comment.
This probably doesn't belong to the game class. It's the map's cache validity this flag denotes. It also should be private otherwise it defeats the purpose of invalidate_map_cache().
There was a problem hiding this comment.
This is important, this MUST be owned by the map.
There was a problem hiding this comment.
I've looked into map class more thoroughly and found out it already had most of necessary functionality (i.e. it is possible to mark certain caches as dirty), so I utilized this to make new function which invalidates all three caches (transparency, floor and outside). Later we need to revisit places where we invalidate cache and only mark certain caches invalid (depending on context), but I think performance improvement should be noticeable already.
src/game.cpp
Outdated
| @@ -524,7 +524,7 @@ void game::init_ui( const bool resized ) | |||
| // Only refresh if we are in-game, otherwise all resources are not initialized | |||
| // and this refresh will crash the game. | |||
| if( resized && u.getID() != -1 ) { | |||
| refresh_all(); | |||
| g->refresh_all(); | |||
There was a problem hiding this comment.
Simply refresh_all() whenever inside the game class.
(Here an all over the PR).
There was a problem hiding this comment.
Thanks. I wanted to make it consistent with other occurrences, but I guess I can do it later and actually by removing g-> inside of game class.
src/game.h
Outdated
| @@ -572,6 +572,10 @@ class game | |||
| int assign_npc_id(); | |||
| Creature *is_hostile_nearby(); | |||
| Creature *is_hostile_very_close(); | |||
| bool is_map_cache_valid = false; | |||
| inline void invalidate_map_cache() { | |||
There was a problem hiding this comment.
There's no need to explicitly inline: everything defined in the header will be inlined anyways (even in cpp, it's a single line assignment of a primitive data type, which is a perfect candidate for inlining).
|
"Manual" cache invalidation usually tends to be error-prone:
It would be better if the class could decide for itself whether is should invalidate its own cache or not, but that requires fixing the broken encapsulation there first. |
I agree manual invalidation is not good (existing tests caught a few cases where I forgot to add invalidation call). Unfortunately, I didn't find a way to implement automatic invalidation yet. |
|
After moving the caching bool into map and maybe some of the other cleanups, I want to merge this as is, it's pretty bad right now. But looking forward to another PR, the way to make it "automatic" is to have the map primitives that edit the map do the invalidation, and ideally invalidate different sub-caches. For example, if an opaque tile is made non-opaque or vice versa it invalidates the transparency cache and everything that depends on it, when a smoke tile moves, etc. When a light source is activated or deactivated, it doesn't touch the transparency cache or FoV, but it invalidates the lightmap. |
ZhilkinSerg
force-pushed
the
z-levels-map-cache-invalidation
branch
from
b75c702 to
027e63e
Compare
Summary
SUMMARY: Performance "Do not rebuild map cache when it is not necessary"Purpose of change
Remove redundant map cache rebuilds during calls to various routines which do not alter map or visibility.
Describe the solution
Mark map cache as invalidated only when necessary (something is possibly changed on the map).
Additional context
There are a lot to optimize, but performance improvement should be noticeable right now with experimental 3-D FOV enabled (player info or pixel minimap toggle should not trigger map cache rebuild).