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.

Sign up for GitHub

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

Jump to bottom

Implemented Proposed EnergyLink 2.0 protocol #3117

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

Implemented Proposed EnergyLink 2.0 protocol #3117

wants to merge 12 commits into from

Conversation

Jarno458
Copy link
Collaborator

@Jarno458 Jarno458 commented Apr 8, 2024 •
edited
Loading

What is this fixing or adding?

A slot number to the EnergyLink Set packets, this way clients that monitor the Datastorage key for EnergyLink can see the slot, and thereby over some time like 10-15 minutes gather some statistics about what players are contributing or draining the most

This change is fully backwards compatible with the current EnergyLink implementation

This is information can be used for bragging rights, but its mostly intended to help balance enerylink across games, where on large multiworlds it can tracked what games add or drain a lot more or less than other and balance their ratio's based on that

How was this tested?

Not yet, its a super simpel change so i dont forsee any issues here, but would love for someone to try EnergyLink on Factorio or Pokemon R/B

@Jarno458 Jarno458 requested a review from Berserker66 as a code owner April 8, 2024 09:19
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Apr 8, 2024
@Jarno458 Jarno458 added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. meta: help wanted Additional review/assistance is requested for these issues or pull requests. labels Apr 8, 2024
Alchav
Alchav approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@Alchav Alchav left a comment

Choose a reason for hiding this comment

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

Not tested, but if it doesn't break anything I'm fine with it.

agilbert1412
agilbert1412 approved these changes Apr 18, 2024
View reviewed changes
Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

Very simple 2-liner. The only reason why this is a core PR is because these two games have their client in core.

worlds/factorio/Client.py Outdated Show resolved Hide resolved
eudaimonistic
eudaimonistic approved these changes Jun 8, 2024
View reviewed changes
Copy link
Contributor

@eudaimonistic eudaimonistic left a comment

Choose a reason for hiding this comment

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

Tested the changes in Factorio and experienced no functional issues, not that any would be expected anyway. It's a pretty simple change.

@Exempt-Medic Exempt-Medic added the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Jun 20, 2024
@NewSoupVi
Copy link
Member

Looks good but this probably warrants a docs change right? So people know they should implement this into their remote clients as well

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed meta: help wanted Additional review/assistance is requested for these issues or pull requests. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 28, 2024
@Jarno458
Copy link
Collaborator Author

Jarno458 commented Aug 8, 2024 •
edited
Loading

Looks good but this probably warrants a docs change right? So people know they should implement this into their remote clients as well

Well there are no docs about energy link, i created those separatly in #2244, the scope of 2244 is bigger tho it covers a lot of data storage topics

@Jarno458 Jarno458 added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Aug 8, 2024
@Jarno458 Jarno458 added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Aug 8, 2024
@NewSoupVi
Copy link
Member

NewSoupVi commented Aug 8, 2024 •
edited
Loading

I actually wonder now if there should just be a generic MultiServer.py change that makes all datastorage operation replies always report the slot that did the operation

That would have the same effect, but clients wouldn't need to make any changes to support it

(Apologies if that's already part of 2244, I have not looked at it)

@NewSoupVi NewSoupVi mentioned this pull request Aug 8, 2024
Open
@NewSoupVi
Copy link
Member

NewSoupVi commented Aug 8, 2024 •
edited
Loading

Like, how about this? https://github.com/ArchipelagoMW/Archipelago/pull/3747/files
If I understand things correctly, this makes this available to all datastorage keys, not just EnergyLink, and also makes it so clients don't have to make any changes

@Exempt-Medic Exempt-Medic removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eudaimonistic eudaimonistic eudaimonistic approved these changes

@Alchav Alchav Alchav approved these changes

@agilbert1412 agilbert1412 agilbert1412 approved these changes

@Berserker66 Berserker66 Awaiting requested review from Berserker66 Berserker66 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Jarno458 @NewSoupVi @agilbert1412 @Alchav @eudaimonistic @Exempt-Medic