Conditions with math time_since and undefined variables

[MikasaTanikawa]

Describe the bug

After #70996 some conditions may not work properly if condition intentionally uses undefined variable. For example meteorologist now not giving missions because of this. Before #70996 "npc_compare_time_since_var" returned false for undefined variable regardless of operands and conditions like "not": { "npc_compare_time_since_var": "meteorologist_wait", "type": "time", "context": "mission", "op": "<", "time": "2 days" } would return true. Now math time_since returns -1 for undefined variable and uses it to evaluate expression with operands, rewriting this condition as "math": [ "time_since(n_time_mission_meteorologist_wait)", ">=", "time('2 d')" ] returns false.

So porting npc_compare_time_since_var with "<" or "<=" operands should also check if variable is defined?

Attach save file

MeteoTest-trimmed.tar.gz

Steps to reproduce

1. Ask meteorologist some mission.
2. Meteorologist will not give any.

Expected behavior

Port npc_compare_time_since_var to math properly.

Screenshots

Versions and configuration

• OS: Windows
  • OS Version: 6.1 7601
• Game Version: 0d9bfb5 [64-bit]
• Graphics Version: Tiles
• Game Language: English [en]
• Mods loaded: [ Dark Days Ahead [dda], Disable NPC Needs [no_npc_food], Portal Storms Ignore NPCs [personal_portal_storms], Slowdown Fungal Growth [no_fungal_growth] ]

Additional context

No response

[andrei8l]

So porting npc_compare_time_since_var with "<" or "<=" operands should also check if variable is defined?

No, you should use math tools instead of trying to pile secret functions on operators.

For example, you can use has_var() as an additional gate

          "condition": {
            "and": [
              { "math": [ "has_var(n_time_mission_meteorologist_wait)" ] },
              { "math": [ "time_since(n_time_mission_meteorologist_wait)", ">=", "time('2 d')" ] }
            ]
          }

or rely on value_or():

"condition": { "math": [ "time_since( value_or(n_time_mission_meteorologist_wait, 0 ) )", ">=", "time('2 d')" ] }

or idiomatically but longer

"condition": { "math": [ "time_since( value_or(n_time_mission_meteorologist_wait, time('cataclysm') ) )", ">=", "time('2 d')" ] }

both are documented in https://github.com/CleverRaven/Cataclysm-DDA/blob/master/doc/NPCs.md

[PatrikLundell]

It seems someone is trying to get people to download some unknown file (two posts above), using various usernames. I'd suggest NOT downloading it unless you have a contained environment where you can safely examine what it is.

[PatrikLundell]

@NetSysFire: You wanted to be notified about the malware three posts above, so here it is. I can also mention I've notified these posts as spam.

[MikasaTanikawa]

For example, you can use has_var() as an additional gate

But that's problem: non such checks was added when npc_compare_time_since_var was ported to math.

I'm suggesting let time_since return NAN for undefined variable instead -1 and let math comparison operators return false when operand is NAN. This will fully restore behavior of npc_compare_time_since_var without need to check variables in json.

[andrei8l]

But that's problem: non such checks was added when npc_compare_time_since_var was ported to math.

Sure. That's my fault. It's easy to fix.

I'm suggesting let time_since return NAN for undefined variable instead -1 and let math comparison operators return false when operand is NAN. This will fully restore behavior of npc_compare_time_since_var without need to check variables in json.

I toyed with this idea when I wrote #70995. But we are working with a long legacy of code with cascading defaults to 0 for undefined or unexpected variables and it'll take a huge overhaul to fix that.

Furthermore, time_since() can be used in many other contexts in math, not just a comparison. Adding a nan to anything would result in a nan and likely break many other things down the line with no clear error path. Maybe some day we can do that, but definitely not now.