Refactor: Remove unnecessary parameter avatar and map

[Brambor]

Summary

Infrastructure "Refactor: Remove unnecessary parameter avatar and map"

Purpose of change

• Continuation of Refactor: Remove unnecessary parameter avatar #76498
• Found dead code again, used git bisect run as described in Refactor: Remove unnecessary parameter avatar #76498 (comment) again with tst code:

  • lines=$( cat src/avatar_action.cpp | grep ramp_move | wc -l )
    
    if [ $lines -eq 2 ]; then
      exit 1
    fi

Describe the solution

Removed all mentions of avatar and map from src/ranged.h and src/avatar_action.h. Replaced with get_avatar() and get_map() in implementations.

Describe alternatives you've considered

Remove more.

Testing

If it compiles, it should be good.

Additional context

Found dead code again. We should have a test for finding dead code.

[PatrikLundell]

I haven't looked at the changes themselves,only the description, but I have misgivings.
I'm not thrilled by coding that buries dependencies deep into the code, hiding them from the users. It can lead to nasty consequences (such as recursive calls to cache handling because an operation transitively called by cache cleanup hiddenly calls the cache cleanup because the cache is still dirty, as a recent crash example). It also makes it harder to improve the functionality later on when you e.g. want to make PC only operations available to companions or other NPCs, or when you want to be able to make calculations on maps that are NOT the official reality bubble.

Based on the commit texts, most of the removals are from avatar_action, where it should be a reasonable assumption that the target is actually the avatar/PC (if that is to be generalized in the future, the operations should be moved to something with a suitable new name, at which time the character can again be made a parameter).
If the target_handling stuff is all directly tied to UI selections, then, again, it's reasonable to assume it's all PC centered (but it would make it messy if you wanted to support control of multiple characters (party control), or jumping between characters (such as e.g. using mind control magic)). Support operations should not be allowed to make such assumptions, as you may want to allow the "AI" to make use of them for characters/creatures.

[Brambor]

I'm not thrilled by coding that buries dependencies deep into the code, hiding them from the users.

It can have nice decoupling effects.

It can lead to nasty consequences (such as recursive calls to cache handling because an operation transitively called by cache cleanup hiddenly calls the cache cleanup because the cache is still dirty, as a recent crash example).

These changes are simple. There is some recursion somewhere, but the change is from passing ref to avatar/map through function parameters to passing ref through get_avatar/get_map in the implementation. It is not likely anything like what you describe can occur. Or can it? Can you source the issue? I would like to learn, sounds interesting.

It also makes it harder to improve the functionality later on when you e.g. want to make PC only operations available to companions or other NPCs, or when you want to be able to make calculations on maps that are NOT the official reality bubble.

I agree, the refactoring would be a bit harder. You need to add the parameter back.

But from the perspective of the user, it makes more sense to me. If there is an avatar& parameter, I ask myself why. Can I pass NPC.as_avatar() or something? No. Only the avatar can be passed. And if only the avatar can be passed, then it is a pointlessly head-scratching parameter. Like passing 1 to a math function, so that the math function knows what 1 is.

When I encountered the map& parameter, I asked myself "Do we support more maps then? Or do I need to make sure to have the right coordinates or something, when calling this function?". No. map& is always the same as what the get_map() provides.

Based on the commit texts, most of the removals are from avatar_action, where it should be a reasonable assumption that the target is actually the avatar/PC (if that is to be generalized in the future, the operations should be moved to something with a suitable new name, at which time the character can again be made a parameter). If the target_handling stuff is all directly tied to UI selections, then, again, it's reasonable to assume it's all PC centered (but it would make it messy if you wanted to support control of multiple characters (party control), or jumping between characters (such as e.g. using mind control magic)). Support operations should not be allowed to make such assumptions, as you may want to allow the "AI" to make use of them for characters/creatures.

I am not sure if all are tied to the UI. But they do only allow the avatar at the moment...

When changing avatar_action.h, I was sure I was on the right track. It felt like avatar_action is a class and the functions are methods of the avatar. Then of course you would remove the avatar& from the parameter list, just like it is omitted in a method calling syntax.

When changing ranged.h I felt like these functions maybe should be moved to a new file avatar_ranged.h into an avatar_ranged namespace, so it is called in the form of the syntax avatar_ranged::X. This combines the benefits of not needing to #include avatar.h and being clear it is an action made only for the avatar. However, this should be done only if it differentiates from the other methods in that file. Such a difference can be that it uses UI (needs user input). To be honest, I didn't check all the functions for that.

I did avoid removing a Character& parameter from functions that are only called with the avatar. Some could be removed, as it is tight to the UI. I decided this to be a strict refactor with no such polymorphic changes.

[PatrikLundell]

The recursion issue I referenced was handled by #74315, which in turn was revealed by #74287 (by fixing a coordinate bug so coordinates weren't treated as always being out of bounds, and then starting to actually find the cache to be dirty in the dirty cache handling call chain, causing the handling to be called recursively). Can't say the solution is pretty, but at least it works to break the recursion.

A case of the map not always desired to be the reality bubble map is #75567. I personally think we need to eventually support multiple reality bubbles (that PR only uses another one temporarily), although "eventually" may well be a fair number of years into the future. Another case of desiring/needing to use a different map was #75020 (although that was just a tinymap).

[kevingranade]

Again, "it can have nice decoupling effects" is not sufficient justification for all this churn, and "cleaner interface" is definitely not it because the interface just exists and you're obscuring it.

I want to see TANGIBLE benefits for this much code change, and there's been nothing forthcoming, so this is turning into a pretty hard no from me.