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

DKC3: Fix List Out of Range Error on Level Shuffle Hint extension #3077

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

PoryGone
Copy link
Collaborator

@PoryGone PoryGone commented Apr 2, 2024

What is this fixing or adding?

In #2820, I moved hint extension from modify_multidata, which was run on a thread, to extend_hint_information, which apparently isn't, and now that it's running on the main thread, it can be seen that it actually crashes at the end from an off-by-one error. It doesn't do that anymore.

(Also I noticed a player-facing name had an underscore and no one has ever commented on it)

How was this tested?

I actually generated with a level shuffle seed, which I apparently failed to do last PR.

If this makes graphical changes, please attach screenshots.

@PoryGone PoryGone added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. affects: release/blocker Issues/PRs that must be addressed before next official release. labels Apr 2, 2024
@PoryGone PoryGone changed the title Fix List Out of Range Error on Level Shuffle Hint extension DKC3: Fix List Out of Range Error on Level Shuffle Hint extension Apr 2, 2024
Copy link
Contributor

@Ixrec Ixrec left a comment

Choose a reason for hiding this comment

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

Never played DKC3, but it seems clear this would successfully ignore the off-by-one error instead of raising an exception.

Though without any context besides this PR itself, this does feel more like a workaround/mitigation than an actual fix. Is the idea that some of these worlds are sometimes unavailable, so it's perfectly correct behavior to end up with an out of bounds index for worlds/levels that "don't exist"? Or is there a "real" bug with hint info that still need a more involved fix?

Copy link
Collaborator

@alwaysintreble alwaysintreble left a comment

Choose a reason for hiding this comment

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

code itself is fine

@@ -294,7 +294,7 @@
blue_region = "Blue's Beach Hut Region"
blizzard_region = "Bizzard's Basecamp Region"

lake_orangatanga_region = "Lake_Orangatanga"
lake_orangatanga_region = "Lake Orangatanga"
Copy link
Collaborator

Choose a reason for hiding this comment

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

since i don't see it mentioned in your PR description is this also a bug fix?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah it was surrounded in () that's why i didn't see it. then i suppose i just need to ask if it's used in code anywhere as a precaution

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LocationName.lake_orangatanga_region is used throughout the code. That is the purpose of this file.

@PoryGone
Copy link
Collaborator Author

PoryGone commented Apr 6, 2024

Never played DKC3, but it seems clear this would successfully ignore the off-by-one error instead of raising an exception.

Though without any context besides this PR itself, this does feel more like a workaround/mitigation than an actual fix. Is the idea that some of these worlds are sometimes unavailable, so it's perfectly correct behavior to end up with an out of bounds index for worlds/levels that "don't exist"? Or is there a "real" bug with hint info that still need a more involved fix?

Rocket Rush is never (and will never be) randomized. It is the 5th level in the final world. Every world has 5 levels.
This is a full solution, not a band-aid.

@PoryGone PoryGone added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Apr 7, 2024
@Berserker66 Berserker66 merged commit 9bbc492 into ArchipelagoMW:main Apr 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: release/blocker Issues/PRs that must be addressed before next official release. is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants