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

KH2: client optimizations and some more qol #4547

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

Conversation

JaredWeakStrike
Copy link
Collaborator

@JaredWeakStrike JaredWeakStrike commented Jan 24, 2025

What is this fixing or adding?

Addressing qwint's suggestions on #4075
Having the random/yaml settings show up in game

How was this tested?

tested what happens if there is no datapackage/if it were an empty dict
read the yaml after generation for the seed and it was formatted correctly

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Jan 24, 2025
@Exempt-Medic Exempt-Medic requested a review from qwint January 24, 2025 17:20
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Jan 24, 2025
Copy link
Collaborator

@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.

just reviewed the changes per my request, don't have context to review the read_bytes update nor the openKH data changes
pure code review

mostly some small formatting requests but the connect_to_game() comment I think should be tested and proven not an issue or addressed

worlds/kh2/Client.py Show resolved Hide resolved
worlds/kh2/Client.py Outdated Show resolved Hide resolved
worlds/kh2/Client.py Outdated Show resolved Hide resolved
worlds/kh2/Client.py Outdated Show resolved Hide resolved
worlds/kh2/Client.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

the context in my previous comment still stands, but all my issues were addressed

@JaredWeakStrike
Copy link
Collaborator Author

JaredWeakStrike commented Jan 29, 2025

PR #4562 broke because it assumes I use decimal numbers instead of hex, changes the 0 to 16 and it gets the right addresses using the int function instead of eval

Copy link
Contributor

@shananas shananas 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 client works fine on Steam version and openkh changes show up correctly in game

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