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

Core: Fix weights file data not being reused #3381

Merged
merged 1 commit into from
May 27, 2024

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

#3380

i forgot about weighted files when i added that delete line apparently. this reverses that checking by just adding the actual valid options to the set instead of deleting weights that we decide are valid.

How was this tested?

generated with 1 custom and a weights file confirming that invalid options still print and custom options from the weights all worked.

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 22, 2024
@alwaysintreble alwaysintreble added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels May 22, 2024
@ScipioWright ScipioWright added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label May 22, 2024
Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

Fixed the issue I was having, tested with a TextChoice

@NewSoupVi
Copy link
Member

NewSoupVi commented May 23, 2024

In roll_linked_settings, we make a deepcopy of weights and return it.

In roll_triggers, we make a deepcopy of weights and return it.

In the main function, we don't. So if the yaml has linked options or triggers, we're working with a deep copy, but if it doesn't, we're not.

This might be why some yamls were unaffected in the big async. It's the ones making use of triggers, which "saved" them from being modified. (Edit: This seems to be confirmed)

That seems wacky and error prone. Maybe we should just always make a deepcopy at the start?
I guess it's fine as lone as every time we modify, we make a copy first, and that would be more efficient. But that behavior has to be known

@NewSoupVi
Copy link
Member

NewSoupVi commented May 23, 2024

After thinking about it, I think I would like it if we could either

  1. always make a deepcopy, at the very start of the roll_settings function
  2. make a function docstring for roll_settings that includes a note along the lines of "The same weights object may be shared by multiple players, so it should not be modified without making a copy first"

Copy link
Member

@NewSoupVi NewSoupVi left a comment

Choose a reason for hiding this comment

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

Change works. I have some thoughts about whether it is the correct change & whether there should be some protection against this in the future (pls read my comments above), but it's probably a bit too sensitive of an issue to discuss forever.

@NewSoupVi NewSoupVi 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 May 23, 2024
@alwaysintreble
Copy link
Collaborator Author

We never deleted the cache previously so I don't think doing so is a good idea when it can be avoided, I just apparently didn't consider this solution when I wrote the original code. Even if we created a deep copy every time I wouldn't trust being able to delete it.

@NewSoupVi NewSoupVi merged commit dfc347c into ArchipelagoMW:main May 27, 2024
16 checks passed
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
sflavelle pushed a commit to sflavelle/Archipelago-tgc that referenced this pull request Jun 20, 2024
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. 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.

3 participants