-
Notifications
You must be signed in to change notification settings - Fork 703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UpdateHint: create_if_not_exists (Allows clients to hint own items in other worlds) #4316
Conversation
On #3523, and also just from many interactions with users and world devs who have in-game hints, I can confidently say that this is a highly requested feature. I'm hoping we can make it happen this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, read through all the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really like the ability of creating hints in bulk (like an array of locations to create hints for)
this is often useful for opening shops and sending hints for everything the player knows at that point
Also this way of creating hints through updates does not allow to silently create hint or to not broadcast hints a 2nd time, like scout locations has
LocationScouts does not allow this either.
That is how this PR works, though. This PR will only create & broadcast the hint if it doesn't already exist. The list thing is fair though. I might separate it out into its own command for that reason, more opinions welcome |
[{ | ||
"cmd": "InvalidPacket", | ||
"type": "arguments", | ||
"text": 'UpdateHint: Desired hint does not already exist. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would remove already
from this scentence
[{ | ||
"cmd": "InvalidPacket", | ||
"type": "arguments", | ||
"text": 'UpdateHint: Must use "unspecified"/None status for items for other players.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for items for other players
is a bit odd, as its items send by other players. not items for those other players
[{ | ||
"cmd": "InvalidPacket", | ||
"type": "arguments", | ||
"text": "UpdateHint: No Permission", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Permission
Might be on purpose but this gives no information on why your getting this response, and if its as part of an implementation error by the client, then it might not help the dev to understand what went wrong
try: | ||
status = HintStatus(status) | ||
status = HintStatus(status_int) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the try/catch for status = HintStatus(status_int)
above the hint = ctx.get_hint(client.team, location_player, location)
as it breaks the flow that there is a check for if hint is None right below it
Decided to go with a new command instead. |
Basically, this is supposed to "supercede" #3523, and the concept of LocationScouts with create_as_hint in general.
I'm not sure if this should be its own new command called
CreateHint
instead.If it were its own command, we could let the client provide a list of locations like with LocationScouts.
However, we would also have to maintain some stuff in two places. In my estimation, anything that you specify when creating a hint is also something you'd want to be able to update.
Lmk what you think.