Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break recursive death spiral #74315

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented Jun 4, 2024

Summary

None

Purpose of change

Break recursive death spiral revealed by #74287.

Describe the solution

Clear the cache dirty flag at the call rather than at the end so calls from rebuild_aim_cache won't call it down the line.

Describe alternatives you've considered

Understand why the code for setting the cache even involves checking for the cache being dirty in the first place.

Testing

Loaded save, spawned m16, wielded it, walked over to zombie miners, aimed. I was able to fire at a miner after aiming one step at a time to full aiming.

Additional context

Note that #74287 didn't cause the bug, just revealed the bug when another one was corrected (the bug can can probably be repeated with the pre #74287 correction code if you're at OMT (0, 0, 0) and the target is in the reality bubble at a position that leaves it with positive absolute coordinate members (x and y above zero)).

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Jun 4, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jun 4, 2024
@Maleclypse Maleclypse merged commit b8f906a into CleverRaven:master Jun 5, 2024
26 of 30 checks passed
@PatrikLundell PatrikLundell deleted the break_spiral branch June 5, 2024 04:27
@osuphobia
Copy link
Contributor

osuphobia commented Jun 5, 2024

Understand why the code for setting the cache even involves checking for the cache being dirty in the first place.

// possibly reduce view if aiming (also blocks light)
if( get_avatar().recoil < MAX_RECOIL ) {
if( get_avatar().cant_see( p ) ) {
return { true, true, 0.0 };
}

Aiming will reduce your visual field, and seems that we are rebuilding aim cache while refreshing the lightmap (and only calling rebuild_aim_cache here).
(Edit:

const bool scope_is_blocking = you.is_avatar() && you.as_avatar()->cant_see( p );

Also calling it here.)
Do you have any idea how we can improve this part?

@PatrikLundell
Copy link
Contributor Author

No, I don't have any knowledge of the code. but there is something wrong with code that risks infinite recursion. The problem is that it's such a long call chain, so it's not merely a matter of factoring out a common part of some code to avoid the risk of recursion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants