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 (-> *autosplit-info-jak1* training-num-orbs) when blacked out #3297

Merged
merged 5 commits into from Feb 29, 2024

Conversation

Zedb0T
Copy link
Contributor

@Zedb0T Zedb0T commented Jan 8, 2024

When loading a save, these values are temporarily 0, and it messes with the autosplitter so we want to ignore them then.

@Zedb0T
Copy link
Contributor Author

Zedb0T commented Jan 8, 2024

(when (not (> (-> *game-info* blackout-time) (current-time)))

Should I change this to (< (-> *game-info* blackout-time) (current-time))?

In my eyes (> (-> *game-info* blackout-time) (current-time)) Is just "in blackout" so I wrote "not in blackout" but idk if it compiles down differently and it would be better to just not have the (not call

@ManDude
Copy link
Member

ManDude commented Jan 8, 2024

It doesn't matter, although the actual inverse of > is <=

@xTVaser
Copy link
Member

xTVaser commented Jan 8, 2024

Readability wise, if you are already confused it's a good sign that it would benefit from a properly named intermediary variable

(let ((in-blackout? (<= (-> *game-info* blackout-time) (current-time))
  (when (not in-blackout?)) ;; or (unless in-blackout?...
    ....

Also is this also a problem in jak 2 (maybe fix it at the same time)?

@xTVaser xTVaser marked this pull request as draft February 17, 2024 18:38
@Zedb0T
Copy link
Contributor Author

Zedb0T commented Feb 21, 2024

Oh I didnt see that you requested changes for Jak 2

Whoops

@xTVaser
Copy link
Member

xTVaser commented Feb 23, 2024

Cleaning up stale PRs - this PR has sat for a while with feedback/conflicts/build failures and no movement, re-open if work resumes.

@xTVaser xTVaser closed this Feb 23, 2024
@Zedb0T Zedb0T reopened this Feb 24, 2024
@Zedb0T
Copy link
Contributor Author

Zedb0T commented Feb 24, 2024

Added the fix to Jak 2 and added the local variable per your request as far as I know this is tested and in a good state to merge, If any other changes need to be made please feel free to let me know.

@Zedb0T Zedb0T marked this pull request as ready for review February 24, 2024 19:49
@xTVaser xTVaser merged commit 6822e5c into open-goal:master Feb 29, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants