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 indexes on printed subs when ice loses all variables subs #7209

Merged
merged 4 commits into from
Nov 12, 2023

Conversation

butzopower
Copy link
Contributor

Fixes #7201
Fixes #6584

The underlying issue for both these is that the subroutine indexes were only being updated via add-sub function, and so when there were 0 calls to add-sub, then the original subroutines stick around but never had their indexes reset. This would cause an issue further in the game during ice encounter in reset-sub which relies on :index being correct to assoc the subroutines back into the list properly after removing broken/fired/resolved status - effectively cloning any subroutines whose indexes were previously greater than or equal to the current number of subs.

This would have caused issues I think only with ice that appends variable subs to the front, so this hopefully fixes issues with Blockchain, Envelopment, and Loki.

@NoahTheDuke
Copy link
Collaborator

Maybe the test should be more explicit about what it's checking? Checking the total number doesn't seem comprehensive.

@butzopower
Copy link
Contributor Author

@NoahTheDuke The original test was more showing how the issue manifested (e.g. only after doing an encounter would the subroutines increase). I've updated the test to check the :index property specifically as that was the issue with the implementation which potentially affects more than just the reset-subs function. I sort of feel it leaks into the test a bit of what should be private engine info (or perhaps we can refactor :index out entirely? it's a bit weird to have to track an items position in a list) but it's definitely more explicit and scoped in terms of what was previously incorrect with the data.

@butzopower
Copy link
Contributor Author

butzopower commented Nov 12, 2023

@NoahTheDuke I've also added a refactor to reset-all-subs that I think does the same thing without worrying about the :index of a sub. I've also removed reset-sub! as it seemed like it was unused. This should hopefully prevent other issues if the index somehow gets messed up.

@NoahTheDuke
Copy link
Collaborator

I checked the originating PR for :index, and it was to allow for calling get-in to get a specific ice without doing a full scan, O(1) vs O(n). This mattered when we're building the log message to sort by the order of the subroutines on the ice.

I think we use it in other ways too now but I don't remember. I would prefer to keep it, especially given that you've caught the couple places I missed initially.

@butzopower
Copy link
Contributor Author

@NoahTheDuke I wonder if that's :index for ICE rather than the :index used on the subs? Or maybe it was for both - it's actually hard to determine from the code unfortunately because they use the same name :/

I think it's fine to keep it around, but I think if we can not rely on it for things like "reset everything in this list" that's probably safer just in case it gets stale.

@@ -78,6 +78,7 @@
new-subs (->> (range total)
(reduce (fn [ice _] (add-sub ice sub (:cid ice) args)) new-card)
:subroutines
(map-indexed (fn [idx sub] (assoc sub :index idx)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be necessary. It's already done by add-ice. Are you sure this is the source of the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - the problem is when Envelopment reaches 0 counters, then reset-variable-subs gets called with total equal to 0 which means the (range 0) reduces with no calls to add-ice, so no subroutines get added but also the previous printed subroutines don't get their indexes reset.

If you remove this line you'll see the test fail with the only sub in Envelopment having an :index equal to 1 (instead of 0).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah, range + reduce can be silly. This is a bit nit picky, but I'd prefer to only call map-indexed if total is 0. Add-sub encompasses everything needed to correctly add a sub, so it's not clear why this call is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if like an add-subs function that takes a list of subs to add which then resets the indexes would be better or something. Like the current way actually just recalculates the indexes n times which also seems pretty weird. I wasn't sure all the places we are using add-sub where it's not in bulk already 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, recalculating the index n times is annoying but I couldn't figure out a way to only update the indexes from new-index onward (which is an easy thing to do in javascript lol).

Add-subs is hard because each sub might have a different index to be placed in which will influence the position of later subs, and subs are added one at a time according to the CR.

@@ -90,6 +91,7 @@
new-subs (->> (range total)
(reduce (fn [ice _] (add-sub ice sub (:cid ice) args)) card)
:subroutines
(map-indexed (fn [idx sub] (assoc sub :index idx)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this re-indexing as I don't think it should be required for this one since I think it's append only.

Comment on lines 222 to 223
[sub]
(dissoc sub :broken :fired :resolve))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to keep the api the same as before. Every sub manipulation function takes ice and sub, so this one being different isn't great.

Given that :index isn't being touched by this function, why shouldn't we updating it on the ice directly? When will resetting a sub change its indexes?

(Same question applies to reset-all-subs too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem arose here when you ended up with something like:

{:subroutines ({:index 1 ...})}

With the associating directly into the subroutines list via list index, this actually did a copy rather than an update, hence why the Trash this ICE sub was being duplicated.

To me the top level function looks like it just wants to clear the resolved/broken/fired flags on all subroutines for the ice, this seems completely like something that should just apply to everything in the list and feels more like a straight up map of something rather than something that has to be concerned about the stored indexes being correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would actually be fine with just inlining and deleting reset-sub as well since it's currently only used in reset-all-subs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Deleting the function is fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool - I still declared it separately via leftfn which names it a bit (it was hard for me in the old code to figure out what was happening) but also because my clojure-fu is weak and I wasn't sure how to do the map function inside the inline function

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect use of letfn.

@NoahTheDuke
Copy link
Collaborator

I wonder if that's :index for ICE rather than the :index used on the subs?

break-subroutine-msg uses the :index of the subroutine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bankhar incorrectly interact with Envelopment Envelopment gains second "Trash this ice" Subroutine
2 participants