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]

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.

[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.

[Maleclypse]

I'm assuming this needs some followup reviews before merge based on what I read above?

[GuardianDll]

No, not really, discussion above speaks about general issues in eoc coordinates, that need much bigger refactoring to be fixed. This one, while uses coordinates, tries to fix pretty different issue

[Night-Pryanik]

#78140 was merged, can you retest your changes as you planned?