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

Stardew Valley: Add tracker text client integrated with UT and commands to explain the logic #4378

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

Conversation

Jouramie
Copy link
Contributor

What is this fixing or adding?

This PR is based on #3596, which has been up for a long time now...

This adds a custom client for Stardew Valley that is integrated with Universal Tracker (need to be installed separately).

This leverages the Stardew rule framework to dig into the rules and explain them. We already had functions to explain the logic, but it was only used to debug test failure. Being able to make the rules human-readable helped us a lot while developing, so making the rules available to users to read seems only natural.

The client comes with 4 commands to explain Stardew's deep logic to end users (see screenshots). Universal tracker is required for the Stardew Valley Tracker client to appear in the launcher.

How does it work?

This uses UT ability to generate a world consistently with the player settings and its ability to build a collection state updated with the current multi world. UT uses it to find what locations are currently in logic, but with the Stardew rule framework, since the rules are not lambda but complete objects, we can easily dig into them and find out why they evaluate to True or False.

How was this tested?

Generate multiple explain for different rule types. Made sure colors are correctly applied. See screenshots.

A version of the current stardew_valley.apworld was released with the tracker packaged in for players to test. https://github.com/agilbert1412/Archipelago/releases/tag/0.5.1-stardew-6.6.5-tracker

If this makes graphical changes, please attach screenshots.

In case you forgot that tomatoes grow in summer
image

Working with UT means using UT commands! You can manually collect stuff and the explains will adapt.
image

Sadly, some rules are not meant for humans...
image

Ahh, yes, that's where the mine is!
image

No need to remember location name with fuzzy matching!
image

Using explain_missing will focus on the rules you're missing, and skip explaining what you already have collected.
image

Jouramie and others added 30 commits June 28, 2024 00:55
@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Dec 15, 2024
@agilbert1412 agilbert1412 self-requested a review December 16, 2024 01:11
@agilbert1412 agilbert1412 self-assigned this Dec 16, 2024
@agilbert1412 agilbert1412 added is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: other Issue/PR is waiting for something else, like another PR. labels Dec 16, 2024
@agilbert1412
Copy link
Collaborator

I haven't reviewed it yet, but I will. Just want to say that this looks friggin amazing, great work!

@agilbert1412 agilbert1412 removed the waiting-on: other Issue/PR is waiting for something else, like another PR. label Dec 16, 2024
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.

General approval, few questions/suggestions. Amazing work!



def parse_explanation(explanation: RuleExplanation) -> list[JSONMessagePart]:
result_regex = r"(\(|\)| & | -> | \| |\d+x | \[|\](?: ->)?\s*| \(\w+\)|\n\s*)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any chance this regex could get maybe a comment or some splitting down? Regex are famously unreadable after about 7 seconds after merging

Copy link
Collaborator

Choose a reason for hiding this comment

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

I heard something in dev channels recently about not being allowed to use copyrighted icons in the launcher, but some games are doing it. Can we get a confirmation of is this fair use or not?

If needed, I can provide written permission from ConcernedApe's company to use game assets in non-stardew parts of the randomizer

@@ -25,19 +57,25 @@ def __post_init__(self):
self.sub_rules = []

def summary(self, depth=0) -> str:
if self.mode is ExplainMode.CLIENT:
depth *= 2

summary = " " * depth + f"{str(self.rule)} -> {self.result}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somehow it makes me a bit uneasy to name the variable the same as the method, with the same casing and all, and Python's lack of formal declaration. Is this considered okay?

Jouramie added a commit to agilbert1412/Archipelago that referenced this pull request Dec 17, 2024
@BadMagic100
Copy link
Collaborator

I was looking at this again for HK inspiration and wanted to point out the conspicuous lack of explain_obtained or similar - a pretty common question class is "tracker says I can get this but I don't understand how," or in other words "what is the easiest way to get this check with what I have". explain does a superset of this, much like it is a superset of explain_missing. in this vein I had been considering for HK (before I knew this pr existed even) the possibility of a feature which would analyze the logic and turn up the minimal set of met requirements

@Jouramie
Copy link
Contributor Author

That does sound like something quite easy to add, as the explain function already support that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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