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

Core: Let location name groups work with /hint_location #2814

Merged
merged 3 commits into from
Apr 14, 2024

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

location name groups didn't work with /hint_location. now they do

How was this tested?

ran the command with a group and with a location name

If this makes graphical changes, please attach screenshots.

Screenshot_10

@alwaysintreble alwaysintreble added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. affects: core Issues/PRs that touch core and may need additional validation. labels Feb 12, 2024
@ScipioWright ScipioWright added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 13, 2024
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

will ctx.all_location_and_group_names[game] always be populated if ctx.all_location_and_group_names is populated?
or was the previous elif passing in 'game' just to get some result back from the method?
it seems like from the subsequent 'else' that we would want to confirm the game exists.

Else, the handling of location groups looks good, and did some testing on Doom 1993 hinting using the text client (ex. !hint_location Tower of Babel (E2M8)) and it

  • hinted a random location in the group and deducted hint points for one hint
  • prompted me that there were more hintables
  • on repeat hints gave me new hints until i ran out of points
  • and then with more points hinted until i ran out of locations in the group, and then repeated the relevant hints while deducting no hint points

as expected.

@alwaysintreble
Copy link
Collaborator Author

will ctx.all_location_and_group_names[game] always be populated if ctx.all_location_and_group_names is populated? or was the previous elif passing in 'game' just to get some result back from the method? it seems like from the subsequent 'else' that we would want to confirm the game exists.

the previous behavior was only looking at the location names of the game and didn't include the groups. the second elif checking for the game first is because of older multidata that might not have location_name_groups. the game will always be in the "all" group,

Else, the handling of location groups looks good, and did some testing on Doom 1993 hinting using the text client (ex. !hint_location Tower of Babel (E2M8)) and it

  • hinted a random location in the group and deducted hint points for one hint
  • prompted me that there were more hintables
  • on repeat hints gave me new hints until i ran out of points
  • and then with more points hinted until i ran out of locations in the group, and then repeated the relevant hints while deducting no hint points

as expected.

!hint_location already works on current release, this is specifically for /hint_location from the server/admin, which was missed when implementing location_name_groups

@qwint
Copy link
Contributor

qwint commented Mar 12, 2024

ah beans, will retest tomorrow sometime using the right commands

But I don't think you understood my first comment
elif self.ctx.location_names_for_game(game) is not None:
became
elif self.ctx.all_location_and_group_names:
so it is no longer checking that the game is a valid key for the lookup table (even though the lookup table changed), I'm not certain that is necessary, but the 'else' it falls back on mentions that the game is unknown and that the user should hint by ID instead.

            else:
                self.output("Can't look up location for unknown game. Hint for ID instead.")
                return False

this looks to me like the game could be unknown to the server and could fail to lookup. and if that is true we'd want to fail on the right level to get the right error message.

@alwaysintreble
Copy link
Collaborator Author

this looks to me like the game could be unknown to the server and could fail to lookup. and if that is true we'd want to fail on the right level to get the right error message.

ah i see yeah i did misunderstand. i don't think that code path can actually be hit, since the client's game has to be in the current multiworld's games in order for them to even connect to a slot, but it's probably better to not remove fallback in case that changes.

Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

properly retested with server-side /hint_location Player1 {location_group} hints, and all avaliable hintables in a group would be hinted and/or displayed as expected

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Merged into main and tested that hints now worked with location groups

@Exempt-Medic Exempt-Medic 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 Mar 28, 2024
@Berserker66 Berserker66 merged commit 98e2d89 into ArchipelagoMW:main Apr 14, 2024
15 checks passed
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 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: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. 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.

5 participants