Allow talk effects to be conditional

[haveric]

Summary

Infrastructure "Allow talk effects to be conditional"

Purpose of change

Allow some effects to fail, such as u_spend_cash. This will cancel any future effects. Main goal is to prevent getting things for free from trading during dialogue.

• Update existing references to put u_spend_cash (and other conditional effects) first
• Update docs for which effects can be conditional
• Possible fix, but needs testing or updating: Rubiks 'help me install CBMs' option has some trade-based oddities #54211 After investigating, this is out of scope for this issue.

(Might fix other issues, but I can't find any right now even though I thought some existed for free trading or canceling trading)

Describe the solution

Make talk effects conditional and bail out if the condition fails.

Affects the following effects:

u_buy_item - Fails if you can't afford it or cancel the transaction
u_sell_item - Fails if you don't have the item
u_spend_cash - Fails if you cancel out of the transaction
u_buy_monster - Fails if the npc doesn't sell monster or you can't afford the monster
u_location_variable/npc_location_variable - Fails if no location can be found
u_message/npc_message - Fails if the alpha talker is not the player
npc_set_goal - Fails if no goal can be found
open_dialogue - Fails if the alpha talker is not the player
take_control - Fails if the alpha talker is not the player
u_cast_spell/npc_cast_spell - Fails if there is no caster
u_spawn_monster/npc_spawn_monster - Fails if there's no valid monster to spawn

Describe alternatives you've considered

• Let somebody else implement this
• Alternative method for making only some of these conditional? I'm open to suggestions here
• Extend this, so you can do something different if it fails (I'd probably need help implementing this)

Testing

Tested as part of #55690.
Example:

"effect": [
  { "u_spend_cash": 200000 },
  { "mapgen_update": "tacoma_commune_woodcutter_10_logs", "om_terrain": "ranch_camp_67" },
  { "u_message": "[NPC NAME] drops the logs off in the garage…", "type": "good" },
  { "npc_add_effect": "currently_busy", "duration": "24 h" }
],

If you don't spend the cash by canceling out of the trade window, none of the other effects occur now.

Additional context

None

[Ramza13]

I understand what you are going for but I'm a little concerned about it being the default behavior.
Up until now the idea with EOC's has been to fail quietly as needed because some times a monster attack hits another monster rather than a character or a global EOC runs on NPC's in addition to the player and just having the message not work is fine. I'm not in love with that paradigm but I couldn't think of a better way to do it.
Maybe add an optional parameter to determine whether a fail quits out or not? So by default everything will run but in the cases you want it to fail it fails. I could also be overthinking this, it doesn't come up that often and my gut feeling is that the current way it works is desirable as often or possibly more than the new one.

[haveric]

about it being the default behavior.

It's only for the effects listed, but my approach is probably a bit heavy handed and I can understand trimming this down, but I think these transactional effects should always fail at minimum:

u_buy_item - Fails if you can't afford it or cancel the transaction
u_sell_item - Fails if you don't have the item
u_spend_cash - Fails if you cancel out of the transaction
u_buy_monster - Fails if the npc doesn't sell monster or you can't afford the monster

I'm open to architectural changes to this, but I'm not sure how that would work. Any suggestions are welcome though.

[haveric]

@Ramza13
Just reviewed all of the changed effects. Let me know what you think:

Definitely need to keep:
u_buy_item - 100% of the time needs to fail, otherwise you get free things
u_spend_cash - 100% of the time needs to fail, otherwise you get free things

Probably keep, but questionable:
u_sell_item - Typically we rely on checking for u_has_items in a condition, but I don't see why this shouldn't also fail
u_location_variable/npc_location_variable - If this fails, anything that tries to use it is going to have a bad time

Revert to always succeed and/or don't matter:
u_buy_monster - All instances of this feel like they should be spawn_monster instead, so not sure how valid failing is here
u_message/npc_message - This one is the most likely that it should never fail as it's used a lot
npc_set_goal - Not used currently
open_dialogue - Only used once, no effects after
take_control - Not used currently
u_cast_spell/npc_cast_spell - Only a couple uses, I don't think this will cause issues, but is used in EOCs
u_spawn_monster/npc_spawn_monster - Should never realistically fail, unless the monster id is invalid

[Ramza13]

about it being the default behavior.

It's only for the effects listed, but my approach is probably a bit heavy handed and I can understand trimming this down, but I think these transactional effects should always fail at minimum:

u_buy_item - Fails if you can't afford it or cancel the transaction
u_sell_item - Fails if you don't have the item
u_spend_cash - Fails if you cancel out of the transaction
u_buy_monster - Fails if the npc doesn't sell monster or you can't afford the monster

I'm open to architectural changes to this, but I'm not sure how that would work. Any suggestions are welcome though.

One way is to add better conditions to prevent this happening, testing for if the player has enough cash for example, I know that's kinda clunky though and won't work for all of them.
Another might be to give these effects some sort of success/fail effects where it runs other effects or EOCs only if it succeeds or fail. Also a bit annoying but makes it fully controllable and optional.

Of the things you changed most make sense, message is the one I'm most opposed to as that one in a lot of cases could be for things that could legitimately happen to an npc or monster with the message only for the player.

[haveric]

One way is to add better conditions to prevent this happening, testing for if the player has enough cash for example, I know that's kinda clunky though and won't work for all of them.

Unfortunately conditions need to happen before the dialogue is shown. Anything using the trade window happens during an effect, hence the need for these changes. I can't think of a better way to deal with these, without some serious infrastructure re-work.

Another might be to give these effects some sort of success/fail effects where it runs other effects or EOCs only if it succeeds or fail. Also a bit annoying but makes it fully controllable and optional.

I considered this, but I think this is beyond my capability at this time. If you (or anyone else) is willing to help add that functionality, that would be welcome.

message is the one I'm most opposed to

Seems reasonable. I'm not super familiar with the usage. I don't think it should ever actually fail from what I've looked at in code, but not being 100% confident, I can totally back out it there.

[Ramza13]

Another might be to give these effects some sort of success/fail effects where it runs other effects or EOCs only if it succeeds or fail. Also a bit annoying but makes it fully controllable and optional.

I considered this, but I think this is beyond my capability at this time. If you (or anyone else) is willing to help add that functionality, that would be welcome.

Ok I will add that.

[haveric]

Another might be to give these effects some sort of success/fail effects where it runs other effects or EOCs only if it succeeds or fail. Also a bit annoying but makes it fully controllable and optional.

@Ramza13 Have you had a chance to look at this yet? If not, I think I have all my other infrastructure work in a decent place, so I have some free time to re-work this PR if you could at least point me in the right direction.

[Ramza13]

Another might be to give these effects some sort of success/fail effects where it runs other effects or EOCs only if it succeeds or fail. Also a bit annoying but makes it fully controllable and optional.

@Ramza13 Have you had a chance to look at this yet? If not, I think I have all my other infrastructure work in a decent place, so I have some free time to re-work this PR if you could at least point me in the right direction.

Bah I'm sorry, I've been working on the moving monsters/items mapgen stuff which is way harder than I thought and put this off. It should be a pretty simple thing, I'm gonna commit to working on it tomorrow or maybe tonight if I can.

[haveric]

That's ok, I figured you were working on a bunch of things already and were just busy. I've found other stuff I can work on for the time being, but this will be an awesome improvement when its complete. (As will your other changes when you get those working).

I certainly appreciate the assistance though, so keep up the great work! I've certainly hit a few of those "harder than you thought" tasks lately, especially getting into this infrastructure work.

I'll keep this in draft for now, but I suspect your work will completely replace it. Feel free to reference or ignore it to your heart's content.