Trying to fall asleep next to a tree is incredibly slow, but waiting is not #73552
Comments
NetSysFire
added
Code: Performance
|
Sorry, I have no idea what might be going on. The only thing I've done is to try to make the buggers show up when they're supposed to (and disappear when the rest of the tree does), but I have no idea what effects they may have beyond that. If it is caused by treetops then rebug removing them with map editing should make a difference. I'd also try to wait first and sleep afterwards (to catch the case when whatever is causing it goes away with time). I might also try adjusting the time to night time with a heavy cloud cover to make sure there is no light to shadow (although that doesn't guarantee the processing is skipped). I'd also try to teleport away to sleep (again to catch the case where something with the character passes with time). |
|
NetSysFire
changed the title
|
Did you spawn the treetop as well, or only the base of it? I suspect the treetop doesn't spawn unless you causes the game to load submaps. |
|
It makes no difference whether or not a tree top is present. |
|
I tried to hack trees to not specify any roofs, but it made no difference (in case there's some weird extra "should there be a roof here" stuff). However, I've encountered a new problem, namely that the game crashes (with the hack reverted), presumably during wildlife movement:
on line |
|
/confirmed I can see what you're reporting, but have no idea what's causing it. |
github-actions
bot
added
(S2 - Confirmed)
|
A few fun facts. To test it out, I found a lonely standing tree.
Note. Character position for further testing
No other ideas. Except for the idea of hooking someone up to see what process is causing the game to slow down. |
|
I found the parameter that's causing the slowdown. It's all about Change it to 0 and compare the speed when you try to sleep. At 10, there's already a noticeable slowdown. 100 and 45, in my opinion, are no different. I haven't really measured it accurately. So I think the reason for this is the coverage parameter. More precisely, in the interaction between this parameter and the function responsible for sleep. But we need someone who can check this and tell us more precisely. UPD. Some changes. If you try to fall asleep on a tree trunk, there is no slowdown. Provided there are no other items with coverage greater than 0 nearby. If you teleport into a tree, there will be slowdown. But because the game forcibly moves you to a neighboring square. Surround the tree with walls and you can sleep without slowdown. Surround the tree with walls and leave one free tile. And you can sleep on it without slowdown. Or place a tree trunk in the house. Similar. There is no slowdown. The bed has a 40% cover. But it has no effect on speed. As long as you don't take the bed outside. |
NetSysFire
added
Fields / Furniture / Terrain / Traps
|
Cover has some functionality in lightmap.cpp and it's possible that the issue is there. Building the vision cache is really resource intensive. |
|
And you probably answered correctly. Cataclysm-DDA/src/lightmap.cpp Line 194 in 35073a9 I was paying attention to that condition: Cataclysm-DDA/src/lightmap.cpp Line 220 in 35073a9 So I went to a tree, sat down and waited for 5 minutes. Which resulted in a similar slowdown of the game. If the character is standing, this condition is ignored. |
|
Well this is weird. The function name implies we're always rebuilding the cache, but we're... only sometimes? In any case, this SHOULD be perfectly safe. And I can confirm that it eliminates this performance issue entirely: index bbe88a5b40..f73b3fc18d 100644
--- a/src/lightmap.cpp
+++ b/src/lightmap.cpp
@@ -219,8 +219,10 @@ bool map::build_vision_transparency_cache( const int zlev )
vision_transparency_cache[p.x][p.y] = LIGHT_TRANSPARENCY_OPEN_AIR;
} else if( ( is_crouching || is_prone || low_profile ) && coverage( loc ) >= 30 ) {
// If we're crouching or prone behind an obstacle, we can't see past it.
- vision_transparency_cache[loc.x][loc.y] = LIGHT_TRANSPARENCY_SOLID;
- dirty = true;
+ if( vision_transparency_cache[loc.x][loc.y] != LIGHT_TRANSPARENCY_SOLID ) {
+ vision_transparency_cache[loc.x][loc.y] = LIGHT_TRANSPARENCY_SOLID;
+ dirty = true;
+ }
}
}
|
|
Is the problem exactly in this function? Maybe I misunderstood something. As I understand it:
In the tree example, only for one tile the vision_transparency_cache parameter is updated. And it doesn't explain why it doesn't happen in a house. Or if you surround the tree with walls. Perhaps we should look at a function that uses this value? |
|
It sets the seen_cache as dirty every turn: Lines 9560 to 9562 in 6a8d75a Which then triggers the rebuild of the seen cache. which is the entirety of the hot path during profiling: Lines 9580 to 9582 in 6a8d75a I don't see any reason to set the cache as dirty if the cache does not change. Notably the seen cache is already being properly dirtied if the player moved or their vision range changed. Line 9579 in 6a8d75a So the code I'm suggesting changing is explicitly resetting the cache every turn even when the situation doesn't change. Actually, we can be even safer here by rebuilding the cache if movemode changes index bbe88a5b40..7e5d283ff6 100644
--- a/src/lightmap.cpp
+++ b/src/lightmap.cpp
@@ -212,6 +212,7 @@ bool map::build_vision_transparency_cache( const int zlev )
bool low_profile = player_character.has_effect( effect_quadruped_full ) &&
player_character.is_running();
bool is_prone = player_character.is_prone();
+ static move_mode_id previous_move_mode = player_character.current_movement_mode();
for( const tripoint &loc : points_in_radius( p, 1 ) ) {
if( loc == p ) {
@@ -219,8 +220,11 @@ bool map::build_vision_transparency_cache( const int zlev )
vision_transparency_cache[p.x][p.y] = LIGHT_TRANSPARENCY_OPEN_AIR;
} else if( ( is_crouching || is_prone || low_profile ) && coverage( loc ) >= 30 ) {
// If we're crouching or prone behind an obstacle, we can't see past it.
- vision_transparency_cache[loc.x][loc.y] = LIGHT_TRANSPARENCY_SOLID;
- dirty = true;
+ if( vision_transparency_cache[loc.x][loc.y] != LIGHT_TRANSPARENCY_SOLID ||
+ previous_move_mode != player_character.current_movement_mode() ) {
+ vision_transparency_cache[loc.x][loc.y] = LIGHT_TRANSPARENCY_SOLID;
+ dirty = true;
+ }
}
}
|
|
i am still not very familiar to what happens here, but i tested the last suggestion, and what i spot is that it doesn't help to solve the bug in any way, comparing to the first diff |
|
What about crafting? When a flashlight (the only source of light in a dark room) runs out of batteries the character should no longer be able to see to continue crafting. Will the character stop crafting with these changes? |
I'm sure I'm misunderstanding something. But doesn't it look like the following? I'm confused about a few things, but the main one is: This condition is always false. Other things:
As far as I understand, the character cannot change its movement mode during the execution of this function. |
https://en.cppreference.com/w/cpp/language/static Basically the value held inside the static variable is retained for next time the function is run.
The specific for loop I suggested modifying only does the 9 squares around the player's XY coordinates, on the z-level it was called for. However the call to map::build_vision_transparency_cache calls it across all z-levels, sequentially. It only checks valid z-levels.
C++ operator precedence means that it can return early. It is an extraordinarily cheap check compared to rebuilding the entire vision cache, so regardless... yeah. |
Oh and that's why it doesn't work in Guardian's testing lol, it needs another assignment or else it's stuck on the very first value it picks up forever. |
|
I still continue to not understand. Lines 9560 to 9562 in 6a8d75a |
|
|
|
I had help from @andrei8l when I messed with vision, perhaps they can weigh in (sorry for the ping if not!), as they seem to know their stuff. |
|
vision_transparency_cache and transparency_cache can be different but still be in sync. That memory copying overhead can be eliminated by updating vision_transparency_cache only when transparency_cache is dirty, player's movement mode changes, or the map coverage changes ( I believe that ter/furn/veh changes already set transparency_cache to dirty, so maybe not need to check coverage separately). |
|
On current master, I can try to sleep adjacent to a tree with seemingly no slowdown. Time advances in 5 minute steps while trying to fall asleep, 2-3 steps per second for me, a few seconds for the full half hour til "You have trouble sleeping". Can you provide a more specific reproduction steps, or a simple unmodded save demonstrating this? |
|
@sparr Is the save in the OP not sufficient? I re-profiled it, it's still got the same issue. My linked PR which initially closed this issue might have shuffled around the exact hot path-ness, but it didn't end up resolving anything.
|
|
Discussion here and Discord and in #77026 suggests this problem might be specific to Windows / MSVC |
|
Using the linux tiles build off github which is compiled using gcc9 on I think ubuntu 20.04 or 22.04, also on linux, has that issue for me which I just reproduced again. As such I doubt this is windows specific. |
|
The problem in #77026 is the same as this one. My attempt at profiling that one points at the same culprit(s) as @RenechCDDA has found above. When it comes to specific OS/compilers, maybe it's the opposite, i.e. there may be ones that somehow are not seriously affected. Regardless, it's something that probably needs to be addressed (once someone can figure out how). |
|
Okay, I'm not afraid to make a fool of myself. Cataclysm-DDA/src/lightmap.cpp Line 198 in 3841a67 Do I understand correctly that Cataclysm-DDA/src/lightmap.cpp Line 228 in 3841a67 But these changes are only made to the copy, not the original. Shouldn't there be something along the lines of: Somewhere before And I'll ask again, why do we call this function, for each z level. Lines 9560 to 9562 in 6a8d75a If for all levels except the one the character is on. The function does nothing? Cataclysm-DDA/src/lightmap.cpp Lines 205 to 207 in 3841a67 I understand correctly that return interrupts the execution of the function, don't I?
Note. Isn't that why we have a theme: #77112 I'd like to remind you: Cataclysm-DDA/src/lightmap.cpp Line 217 in 3841a67 if ...Cataclysm-DDA/src/lightmap.cpp Line 226 in 3841a67 They're inside the same function. How can they not coincide? If the value setting and the comparison take place in the same call. |
|
|
Yes, to me it looks like could be replaced by And, again, yes, I can't see the loop changing player_character, so there shouldn't be any movement mode changes. Thus, it seems |
|
@PatrikLundell there's a reason to rebuild all vision transparency caches: #72542 FYI |
|
I would definitely say any attempt to resolve this should involve reverting #73590 |
|
If there's a reason to rebuild all caches, then they should all be rebuilt (which may well be what reverting #73590 would accomplish). And I would guess it won't help with the performance, but fast but incorrect is rather useless anyway... |
Okay, hold on. We call the Lines 9560 to 9562 in 6a8d75a Inside the function, we update the cache. Based on what we specified earlier and #72542, it's this line: Cataclysm-DDA/src/lightmap.cpp Line 198 in 3841a67 Right? But I don't see a Cataclysm-DDA/src/lightmap.cpp Lines 205 to 206 in 3841a67 I didn't miss anything? |
|
The fetching of the map_cache requests the one for the specified level, so yes, it's one cache per level. By the way, if we are to update all levels then there's no reason to loop outside of the operation (in its single user), as that can be done internally. Edit: If the cache should be processed on all levels, the crouching stuff should still only be done only on the current level, as it uses the PC's position, and I don't think it makes sense to set the caches on other Z levels to the same as you'd have on the surrounding tiles (on the same level). However, it should be noted that the code does indeed do something on other Z levels, due to the memcpy call to copy the contents on the transparency_cache onto the vision_transparency_cache, even if it bails out after that, so I'm wrong about it not doing anything in these cases. You could also change the loop over points_in_radius to first check for crouching etc. and only do the PC tile if not, and then only check for coverage in the loop. That might actually speed things up in our problem cases, because I don't think the PC is actually lying down to sleep, and thus won't have to process the surrounding tiles. Edit 2: |
|
Another stupid question. How many loops are in this line? 9 or 27? A position consists of 3 coordinates, each can have 3 values (radius 1). Cataclysm-DDA/src/lightmap.cpp Line 219 in 3841a67 A little further, the radius can be up to 60. How many cycles is that? 120^3? Cataclysm-DDA/src/lightmap.cpp Line 235 in 3841a67 UPD. I'll explain why I'm fixated on this point. The game slows down not only when you sleep (prone position). But also when just waiting, provided the character is lying down (prone) or sitting (crouching). So the cause must have something to do with it. |
|
WRONG!!! However, it seems we are in need of a points_in_radius_2D operation, because that's what we actually want here, and probably in a lot of other places too. This seems to actually be the root of the issue.~~ Correction. |
|
I would suggest adding a debug message somewhere here: Cataclysm-DDA/src/lightmap.cpp Line 225 in 3841a67 if( vision_transparency_cache[loc.x()][loc.y()] != LIGHT_TRANSPARENCY_SOLID ||
previous_move_mode != player_character.current_movement_mode() ) {
previous_move_mode = player_character.current_movement_mode();
vision_transparency_cache[loc.x()][loc.y()] = LIGHT_TRANSPARENCY_SOLID;
dirty = true;
++ debug message (...)
}Then see how many times this debug message appears in 10 seconds. Provided the character does not change his position. And is in a lying position next to any object that has a coverage greater than 30. That is, it satisfies the condition: Cataclysm-DDA/src/lightmap.cpp Line 223 in 3841a67 This should confirm or deny the assumption that Because if it returns true every turn, then there is a cache update every turn. Line 9582 in 6a8d75a And another stupid question. The Cataclysm-DDA/src/lightmap.cpp Lines 1005 to 1006 in 3841a67 That's probably normal, but I'll ask just in case. UPD. Deleted. |
|
We've been barking up the wrong tree! The reason this is slow isn't because this particular code is slow, but rather because of Thus, the problem to solve would be to detect that the vision transparency cache actually hasn't changed, and thus that the cache isn't actually dirty.
|
|
build_seen_cache has 4 parameters with default values, so not providing the last three is OK, as the defaults are used. Some nutjob has decided that it's good practice for C(++?) to not display defaulted parameters in the implementation, but only in the declaration because providing the full profile is somehow cluttering the implementation presentation. Thus, you can never trust the implementation to show the full picture. There are checks in place that rejects your code if this info is provided. Edit: |
|
|
|
Thanks for the clarification |
|
OK, this is essentially the same as the version posted above, with various minor tweaks that shouldn't affect functionality (but might be very marginally faster): I can make a PR of this, unless @IdleSol wants to do it. I've subjected it to some minor testing:
It can be noted that this only reverts the part of #73590 that doesn't do anything (comparing the PCs stance to a copy of the same stance. If anything, the change doubles down on that PRs approach of not dirtying the cache if nothing has changed by comparing to the previous cache state. My understanding is that this code copies the current transparency cache, presumably just evaluated, and then adjusts it by blocking visibility through translucent terrain (light through, but not vision: stained/frosted glass, etc.), and obstructions due to stance. The vision cache should only be marked as dirty if these additions change the vision cache state, but not if it retains the state from the previous turn. Any changes caused by changes to the transparency cache should be dealt with elsewhere, as there's nothing in this code dealing with that. |
|
I definitely don't want to. I mean, I don't even understand half of this stuff |
|
Fixed in #77193. |
Describe the bug
There are no nearby monsters either.
I ran the game in gdb to see what it was doing:
Weird. I tried sleeping outside of the improvised shelter but no dice. Wild guess: treetops? I know these would do z-level shading things. And indeed going just a bit west into the open makes this issue go away.
Attach save file
Innawoods-trimmed.tar.gzReproducible by trying to sleep adjacent to a tree.
Steps to reproduce
(While the save is obviously an innawoods one, I highly doubt this is mod specific)
Expected behavior
Falling asleep progresses as fast as waiting under tree tops.
CC @PatrikLundell since you did a lot of tree tops related things and might know whats going on here.
Screenshots
No response
Versions and configuration
Dark Days Ahead [dda],
Disable NPC Needs [no_npc_food],
Portal Storms Ignore NPCs [personal_portal_storms],
Slowdown Fungal Growth [no_fungal_growth],
Innawood [innawood]
]
Additional context
No response
The text was updated successfully, but these errors were encountered: