Skip to content
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

MultiServer: CreateHints command (Allows clients to hint own items in other worlds) #4317

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Dec 1, 2024

Supercedes #4316

Basically, this is supposed to supercede the concept of LocationScouts with create_as_hint in general, and have additional functionality.

Thanks to Jarno458 and EmilyV99 for their reviews on #4316, as well as Scipio's, alwaysintreble's and EmilyV99's (again) reviews on #3523. I apologize that your review work is lost, finding the right design is an iterative process that can only be done with reviewers' help :)

One question I anticipate is:
The code looks really similar to LocationScouts, so what was wrong with the original PR that allowed off-world LocationScouts?

The difference here is that this PR doesn't send back a LocationInfo packet, meaning that it can't be used to find your items in other slots. In fact, this packet will actually completely fail if any of the locations specified don't contain an item for the requestion slot, or the location doesn't even exist in the first place. Clients trying to find their remote items by scouting every slot one by one with their respective entire datapackage would've been very bad, but also very appealing.

@github-actions github-actions bot added the affects: core Issues/PRs that touch core and may need additional validation. label Dec 1, 2024
@NewSoupVi NewSoupVi added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Dec 1, 2024
MultiServer.py Show resolved Hide resolved
MultiServer.py Outdated Show resolved Hide resolved
MultiServer.py Show resolved Hide resolved
Copy link
Collaborator

@BadMagic100 BadMagic100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test it but this looks good. Assuming it is merged and once the client supports it I would want to migrate to use this exclusively rather than location scouts with create_as_hint.

I think it would be more correct in theory to pass a list of {location: int, status?: HintStatus} (typescript type here for brevity) but I can't think of the reasonable/common use case where the priority is actually different per location and the caller can send multiple packets if needed as a workaround

@Jarno458
Copy link
Collaborator

Jarno458 commented Dec 1, 2024

Personally for the case of the player gaining a lot of knowledge in bulk i would love to have a way to like hint 25 locations (like i do Satisfactory) without spamming the user
It currently do it by two location scounts, one to know what there without creating hints, and the 2nd to create as hint with only the progression items
Maybe create hints could support creating hints silently?

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Dec 2, 2024

Personally for the case of the player gaining a lot of knowledge in bulk i would love to have a way to like hint 25 locations (like i do Satisfactory) without spamming the user It currently do it by two location scounts, one to know what there without creating hints, and the 2nd to create as hint with only the progression items Maybe create hints could support creating hints silently?

I don't think I wanna add that to this specific PR (I'd like to keep this initial PR somewhat "minimal"), but it's an idea we could discuss for expanding it

@NewSoupVi NewSoupVi added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Dec 2, 2024
@NewSoupVi NewSoupVi requested a review from Jarno458 December 2, 2024 06:33
@NewSoupVi NewSoupVi added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Dec 2, 2024
@NewSoupVi NewSoupVi removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants