MultiServer: Fix LocationScouts with "only_new" broadcasting hints for found locations over and over

[NewSoupVi]

This PR makes it so hints created with LocationScouts create_as_hint = 2 always persist, so that they do not get rebroadcasted if the packet gets sent again.

History:

In #4214, we changed the behavior of LocationScouts with create_as_hint=2 to fix an issue where already found locations would still broadcast every time they were scouted.

However, this had the unintended consequence of making !hint Power Star not just broadcast every previously found Power Star, but also make all of those hints persist.

As a result, we reverted the change in #4367.

However, this brings back the old problem as well. There were two proposed solutions to the issue, I have chosen to go with this one. For the record, the other one is "don't broadcast nor create hints for already found locations, even in the case of LocationScouts"

[NewSoupVi]

The only thing that's awkward here is that only_new is kind of badly named now. It might be good, as Berserker suggested in Discord, to have only_new and remember_everything as two separate behaviors, and make create_as_hint a flag.

[qwint]

conceptually i like it but will need to have more extensive testing than the original pr that ended up accidentally breaking things
(also you have conflicts already smhmh)

[NewSoupVi]

it will need to have more extensive testing

Let's specify what that means

Since this adds an or to a condition, the only time there is a change from current is if hint.found is False, but only_new is True.
This means that when it comes to safety, we need to investigate code paths and use case where only_new is True or becomes True at some point.

That much I am confident in. I won't prime reviewers about anything else.

[NewSoupVi]

Btw, I would specifically also like some opinions about making this a new input param for the notify_hints function called only_found or persist_even_if_found, that is set during LocationScouts regardless of whether create_as_hint is 1 or 2.

[qwint]

did a mix of create_as_hint 0, 1, 2, and !hint in a text client
for unchecked locations:
0 never sent a message but always returned the relevant info
1 always sent a message as expected
2 sent a message if the hint wasn't already printed jsoned (by !hint or by create as hint 1 or a previous create as hint 2)

for checked locations:
0 never sent a message
1 always sent a message as expected
2 sent a message if the hint wasn't already printed jsoned by create as hint 2 but when used after create as hint 1 or !hint create as hint 2 would still send a message

!hint everything seemed to re-send all hints (found or not) to other players (the relevant ones)

I think the create as hint 2 not respecting hints on found locations from other sources is a bit weird, but at least it stops duplicating at some point
but wasn't 'not sending found hints requested by the user to other users` part of the thing we wanted to fix?

[NewSoupVi]

I think the create as hint 2 not respecting hints on found locations from other sources is a bit weird, but at least it stops duplicating at some point but wasn't 'not sending found hints requested by the user to other users` part of the thing we wanted to fix?

In the case of !hinting something found, then scouting it, it's correct that you get the hint printed twice. The first was an intentionally non-persistent type of hint, the second was (and the reason for its persistence is unrelated - It's because scouts should have a complete record)

However, I'm not so sure about create_as_hint = 1 followed by create_as_hint = 2. I wonder if this PR shouldn't reuse only_new, but rather use a new param force_persist that's used by all scout-hints. I think you might have suggested this before as well?

[NewSoupVi]

Ok, gonna update the PR, I think I will just make this a new attribute.

[NewSoupVi]

Ok, I think I like this solution because it gives the client devs the maximum amount of options for different use cases

Devs of games with "shop"-like scouts, where a finished multi should have a complete catalogue of all scouted shop items, can just send create_as_hint = 2 and forget about the rest.
Devs of games with "gossip stone"/"hint-tile"-like scouts, where you'd rather already found items just not be sent at all, can prevent sending scouts for already checked locations client side.

We could even add a new flag to this called "broadcast_even_if_found", then we'd have all 4 behaviors:

create_as_hint=1 + broadcast_even_if_found=False: Rebroadcast only unfound scouts over and over.
create_as_hint=1 + broadcast_even_if_found=True: Rebroadcast all scouts over and over.
create_as_hint=2 + broadcast_even_if_found=False: Broadcast only new hints for unfound locations.
create_as_hint=2 + broadcast_even_if_found=True: Broadcast only new hints, regardless of found/unfound.

But I might add that to CreateHints instead tbh, after we (hopefully) merge this PR.

[qwint]

you're telling me i gotta do all that testing again? :p

[NewSoupVi]

Yes :)