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

Fix IsRelatedToActivePlayer logic for itemlink groups #99

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

BadMagic100
Copy link
Contributor

@BadMagic100 BadMagic100 commented Jul 20, 2024

Well, this has been discussed already what the issue is, in short IsSharingGroupWith tells you if you have an itemlink in common with a real player rather than telling you if a given player is either you or an itemlink. So, this makes the following changes:

  1. Implement value equality for PlayerInfo based on team and slot (I am trusting visual studio to generate a correct GetHashCode)
  2. Implement IPlayerHelper.ActivePlayer. Since we do plenty of comparisons with active player I felt it was useful to have. Also outside of this implementation, there were a few times I wanted it for my own usage in HK.
    • This allowed the removal of ConnectionInfo from player-related packets. So I did that. All of the constructors are internal so this is non-breaking.
  3. Obsolete IsSharingGroupWith - it does what it says it does but that is not a helpful thing to do. So I would advocate to remove it in the future.
  4. Add GroupMembers to PlayerInfo. Annoyingly there was not a very convenient way to get these in the form of PlayerInfo at construction time, but I think all the user-facing API should deal in PlayerInfo in the future. So I just stored the raw slot list and made GetGroupMembers which handles the mapping to PlayerInfo mapping provided a player helper. I would advocate a similar mapping for the Groups (NetworkSlot) in the future
  5. Correct the implementation of IsRelatedToActivePlayer for log messages using Player.GetGroupMembers(playerHelper).Contains(playerHelper.ActivePlayer) or similar (which now works because of value equality on PlayerInfo)
  6. Add IsRelatedToActivePlayer to ScoutedItemInfo. My understanding is that normal ItemInfos should always be related to the active player since they are only received items.
    • I had some doubt whether this should be included in serializable item infos so it is intentionally omitted for now, let me know your thoughts on this
  7. Updated unit tests according to the refactorings done
  8. Added new unit tests for IsRelatedToActivePlayer for both log messages and scouted items

@Jarno458 Jarno458 merged commit a3a661c into ArchipelagoMW:main Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants