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

use local coordinates instead of global in avatar::rebuild_aim_cache() #74287

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

PatrikLundell
Copy link
Contributor

Summary

None

Purpose of change

Fix #74111
Feed relative coordinates into an operation that uses relative coordinate calculations, rather than global coordinates.

Describe the solution

Pass the just computed local coordinates into calc_steadiness rather than the global one fed to rebuild_aim_cache.

Describe alternatives you've considered

Continue to wait for someone to respond to #74111.

Testing

It compiles. I have no idea how to test it.

Additional context

It doesn't seem to be hugely important that most steadiness calculations are against targets extremely far away, but it's possible correcting the code might result in a faster aim when the target is actually considered to be nearby.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels Jun 3, 2024
@Maleclypse Maleclypse merged commit 7147e5d into CleverRaven:master Jun 4, 2024
28 checks passed
@PatrikLundell PatrikLundell deleted the aim_cache branch June 4, 2024 08:13
@osuphobia
Copy link
Contributor

It's causing a segfault when aiming.

@PatrikLundell
Copy link
Contributor Author

PatrikLundell commented Jun 4, 2024

Confirmed.

There is a serious bug lurking here.
Debugging I see it's caught in a recursive loop:
:
cataclysm-tiles.exe!avatar::rebuild_aim_cache() Line 1275 C++
cataclysm-tiles.exe!calc_steadiness(const Character & you, const item & weapon, const tripoint & pos, double predicted_recoil) Line 1180 C++
cataclysm-tiles.exe!calculate_aim_cap(const Character & you, const tripoint & target) Line 1166 C++
cataclysm-tiles.exe!Character::sees(const Creature & critter) Line 11372 C++
cataclysm-tiles.exe!Creature::sees(const Creature & critter) Line 488 C++
cataclysm-tiles.exe!Character::sees(const tripoint & t, bool __formal, int __formal) Line 11338 C++
cataclysm-tiles.exe!map::pl_sees(const tripoint & t, int max_range) Line 808 C++
cataclysm-tiles.exe!map::apparent_light_helper(const level_cache & map_cache, const coords::coord_point<tripoint,5,0,0> & p) Line 694 C++
cataclysm-tiles.exe!avatar::cant_see(const coords::coord_point<tripoint,5,0,0> & p) Line 1255 C++
cataclysm-tiles.exe!avatar::rebuild_aim_cache() Line 1275 C++

Edit:
Written #74315 fixing it (i.e. blocking the spiral, but does nothing about the convoluted code allowing a recursion in the first place).

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 <Bugfix> This is a fix for a bug (or closes open issue) [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.

[Code consistency] Does the avatar.cpp operation avatar::rebuild_aim_cache operation make sense?
3 participants