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

SNIClient: Let clients based on SNIClient monitor packages via on_package method #3093

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

Conversation

TheLX5
Copy link
Contributor

@TheLX5 TheLX5 commented Apr 4, 2024

What is this fixing or adding?

Adds the ability for worlds using SNIClient to monitor/handle packages via the on_package method in their client code.

How was this tested?

Edited my (WIP) MMX3 world to listen to Connected, SetReply and Retrieved packages with EnergyLink as their keys (if applicable) to monitor EnergyLink updates and modify a label accordingly, label got updated so it worked fine. The image below shows the label being up to date with the latest deposit which can be seen on the client lines.

Also tested SMW and ALTTP if their clients broke with my changes and they didn't. Worked fine.

If this makes graphical changes, please attach screenshots.

image

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 4, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Apr 5, 2024
worlds/AutoSNIClient.py Outdated Show resolved Hide resolved
Co-authored-by: Doug Hoskisson <beauxq@users.noreply.github.com>
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.

simple code change, CommonContext side looks perfect, SNIClient change looks reasonable, did no actual testing past code review

Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

LGTM - didn't test

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: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants