split HINT_THE_LOCATION from PRESERVE_SPAWN_OMT flag

[GuardianDll]

Summary

None

Purpose of change

Fix #78191

Describe the solution

Make HINT_THE_LOCATION flag, that duplicate funcitonality of PRESERVE_SPAWN_OMT flag, but is required to hint the location where the item was picked up originally

Testing

It works, but fucks up the map feature for smartphones as per #78191 (comment)

Additional context

Waiting #78140 to be merged to test it once again, in hope it would resolve the problem

[PatrikLundell]

I'm being direct in the review. This is not intended to be offensive, so I hope it's not taken that way.

[PatrikLundell]

If the flag is set but set to zero, something is quite fishy (unless the location actually happens to be in the first OMT, which can happen). The default ought to be tripoint_abs_omt::invalid (although I guess the function actually returns an untyped value, and so the default should be untyped as well, i.e. tritype::invalid), and the value should be validated before use, unless has_var somehow guarantees the presence of a valid value, in which case the extraction operation shouldn't need an explicit fallback (it can default to invalid as part of the function profile).

A nitpick is that I'd check for the hint flag first and the value afterwards, as a flag check probably is cheaper than a value check, and the value always is expected to exist if the flag does, but not the other way around.

[GuardianDll]

nope, has_var() simply count amount of variables with such name and return true if it's bigger than 0, so it just trusts the variable actually exists
i do not have a screenshot, but when tested, smartphones properly carried the coordinates (which is shown in item info with debug mode on), and the fact we got only smartphone reports, and not "all science cards are borked" makes me think it is disagreement in coord type between what item stores, and what eoc expects

A nitpick

Done

[PatrikLundell]

Why would a phone be spawned with this flag? The bug report prompting this tries to get the data without the flag, so something else seems to generate the variable. Adding the flag makes generation of the variable "official", but why should it be, and why was the variable generated in the first place, and what's in it?

I suspect this PR may be trying to solve the wrong problem. If there's a legitimate reason for the phone to get the variable without the flag, a check in the distance display code could simply also require the original flag, and the phone would be excluded by the virtue of not having it.
It could also be that the variable existence check is somehow claiming it to exist when it doesn't causing an attempt to read it that returns the default value.

[GuardianDll]

That's partially a my issue to combine bugfix of two issues at once. Adding the flag was intended way back when original PR was rolled, but i simply forgot about it; later i tried to fix it in #77114, but soon realised eoc can't reach variables written in code, which required entire #77515 to resolve it

so something else seems to generate the variable

Yes, eoc itself has an (accidental) backup solution: for some unclear for me reason using undefined variable resulted in game picking up the character current location instead. It happened to be a good failsafe measure, but is not intentional way to work - map cache is stored only around the location this smartphones were active, which should be represented by variable stored in PRESERVE_SPAWN_OMT

[PatrikLundell]

Why both flags? The code in map.cpp saves the data for both flags, so the hint one ought to include the function of the former, unless the former flag has some separate usage elsewhere (it ought to, or else it would be useless as a separate flag), and I'd rather update those locations to check both flags than to have every JSON entry have both flags. This may affect the documentation as well.

[GuardianDll]

I didn't know which is better, to divide functionality between two flags (it would cause a problem if someone try to use only HINT_THE_LOCATION without PRESERVE_SPAWN_OMT) or duplicate functions (also not good)

[PatrikLundell]

My understanding of your description is that some EOC was using an undefined variable (because the phone lacked the original flag), so it generated this variable with and puts the current PC location in some random coordinate format into it? That sounds like a horrible fallback. Rather than flagging a problem it's hidden away and some random problem occurs down the line.

If it is intended that phones should store a location it should presumably mean the EOC won't generate the variable with random crap, but rather read the variable generated at item creation. However, the "fallback" of hiding the problem on an EOC failure should be removed: if the variable doesn't exist the EOC should fail with an error message saying an expected variable of name so-and-so doesn't exist. If possible, the EOC should fail gracefully rather than crash the program.

[ehughsbaird]

I think it's nicer code if there's only one flag to do this and there's simply a check the ensure that if the hint flag is set the other one is too.

[GuardianDll]

My understanding of your description is that some EOC was using an undefined variable (because the phone lacked the original flag), so it generated this variable with and puts the current PC location in some random coordinate format into it? That sounds like a horrible fallback.

the "fallback" of hiding the problem on an EOC failure should be removed

That is correct, that is horrible, and it was not even intentional, i checked some code, but didn't found at which moment it happens exactly. You can check for yourself, the json https://github.com/CleverRaven/Cataclysm-DDA/blob/master/data/json/effects_on_condition/computer_eocs.json#L49 do not contain any fallbacks, and checking some related eoc functions i was not able to detect anything suspicious either

There is a longstanding issue of eoc having lack of variable declaration, therefore all read variables that do not exist are created ad-hoc with assumed value of zero, so i suspect this coordinate issue is part of it - Andrei talked that they would like it to be different, but it would require to redo a giant amount of code and json

[PatrikLundell]

Good to hear we are in agreement.

The EOC you reference applies the location_variable_adjust effect to spawn_location_omt without verifying that variable exists, which it probably should.

The code (npc_talk.cpp operation f_location_variable_adjust around line 4255 performs the requested manipulation on what's extracted from input_var via get_tripoint_from_var.

get_tripoint_from_var is called with input_var as a parameter and is_npc= false. It then extracts and returns a tripoint_abs_ms from the input_var if it has a value. If it doesn't it generates a debug message and tripoint_abs_ms::invalid, but only if !d_actor (false) is true. Otherwise it returns the tripoint_abs_ms position of d.const_actor(false), which presumably is the PC's position.

This possibly correct, possibly invalid, and possibly PC abs_ms position is then modified as directed and written back into output_var if existing, or input_var if not.

In my opinion, get_tripoint_from_var should generate an error message rather than a debug message, and should do so regardless of whether is_npc is true or not. It should also return invalid regardless, and f_location_variable_adjust should check the result of the value returned, and only perform the operation if the value isn't invalid so we don't get "invalid + (1, 0, 0)" and the like (and underflow on (-1, 0, 0).

It can be noted that the manipulation works when the variable exists despite the incorrect coordinates used, because they're written back the same way as they were read, but it's not pretty (and completely wrong when "defaulted" to a position in a different coordinate system).

Note that if the var contains invalid the distance indication code would have to detect it and probably report it as an error while not displaying anything.