Feature ChoiceManager refactor

fredg1

Member
I think ChoiceManager needs a refactoring.
Currently, it's a huuuge mess: half a dozen switch statements so big you don't even know which one you're in, which led to a lot of the handling being placed in the wrong one (e.g. preChoice or postChoice0)

Moreover, the locations' noncombat_queue is completely wrong. Any choice adventure you get is added to the queue, even those caused by items, making it completely unreliable. It would help to list which choice/option actually cause a choice adventure to enter the queue.
Additionally, listing which choice/option costs a turn would help for things that depend on if a turn spent on a location was from a fight or a noncombat.

What I suggest is to group everything by choice adventure instead.

Thoughts?
 

fronobulax

Developer
Staff member
As a user I don't care whether the code is a bunch of switch statements or nested if statements or some some bizarre lookup involving bit vectors.

If preChoice and postChoice handling is in the wrong place then that ought to be addressed. That said, it would be interesting to have examples of things that are in the wrong place and whether a user would notice.

Seems to me that a choice adventure triggered by an item instead of a location ought not to be associated with a location queue but that opinion is based on a lack of knowledge of game mechanics and uses thereof. Maybe a bug to be fixed?

Could you clarify
listing which choice/option costs a turn

Specifically where is this listing and who uses it - developer, scripter or user?

Is the proposed refactoring required to fix the bugs or is this more of a "since I'm already there" kind of thing?

Since this is potentially large, if there is support for/interest in this, perhaps @MCroft et. al. could develop some tests so that there is more than developer driven testing.
 

MCroft

Developer
Staff member
I know Fred's been thinking about this for a long time and might have some sample code. I definitely like the idea of working on more unit tests for it, if we can nail down a design that we all agree provides value to future-us. As Frono notes, Refactoring is a thing you do to help you maintain the code later.

IIRC, there was some debate about the relative merits of a rearrangement of the switch statement into a different hierarchy versus a separation of the data from the functionality, so that we have a choices.txt file in /data, which is easier to support going forward. The position against was that there were significant exceptions which couldn't be handled in a trivial way, and the counter-position that the 94% that could would make the special ones easier to manage anyway...

I also have a vague recollection of some discussion of exposing the choice data to scripters for whatever nefarious uses they had for it.

So, I think there's a discussion to be had about the approach and the goals. And I think there's things to be gained here if we want to make our codebase easier to maintain.
 

fredg1

Member
The position against was that there were significant exceptions which couldn't be handled in a trivial way, and the counter-position that the 94% that could would make the special ones easier to manage anyway...
One of, I think, the biggest misconceptions during this debate was the weight of the exceptions I was talking about.
The only thing I said (when talking about "common" choice adventures) was that there were "a lot of those, I'll hand it to [my interlocutor]".
I guess it was assumed, at the time, that I meant something big like +90%, but I only meant something somewhere between 40% and 60%...
 

fredg1

Member
Seems to me that a choice adventure triggered by an item instead of a location ought not to be associated with a location queue but that opinion is based on a lack of knowledge of game mechanics and uses thereof. Maybe a bug to be fixed?
This can be fixed by listing in which location the non-combats happen, with "no location" (null) for things like item-generated non-combats.
 

fredg1

Member
Could you clarify

> listing which choice/option costs a turn

Specifically where is this listing and who uses it - developer, scripter or user?
Developers: can modify and access.
Scripters: could maybe access? There are a lot of choice adventure whose queue-entering and turn-costing change based on circumstances, so accessing the list outside of the choice adventure isn't sure to give a reliable result...
Users: could be able to see with the spoilers enabled? Which adventure enters the queue isn't very relevant, and when not all options cost a turn, the spoilers often say it (most of the time, options labelled as "skip adventure" are ones not causing the NC to enter the queue or cost a turn)... And again, if accessed from outside a choice, it's potentially unreliable...
Is the proposed refactoring required to fix the bugs or is this more of a "since I'm already there" kind of thing?
The "preChoice/postChoice" issue doesn't require this refactor, but it definitely highlights the issue mentioned in this thread: locating and fixing these misplaced snippets is, in the current state of ChoiceManager, one gigantic PITA.
For the "when does a choice adventure enter the queue/cost a turn" issue, ChoiceManager is definitely not made for this in its current state.
 

fredg1

Member
If preChoice and postChoice handling is in the wrong place then that ought to be addressed. That said, it would be interesting to have examples of things that are in the wrong place and whether a user would notice.
If you mean in general: most users using the relay browser won't notice these issues.
The "particularity" of preChoice is that it is handled before the request is actually sent, so the only way for someone using the relay browser to notice is if they use the history to go back to a choice adventure, then select an option, or if the request fails to send.
For scripters, it is way easier to suffer from this issue, since they can't tell that they aren't truly in the choice adventure; they need to trust mafia.


If you mean specific instances of this problem, this should be all of them:
Choice adventures 931 and 932 have handling in preChoice that belongs in visitChoice.
Choice adventure 1197 has handling in preChoice that belongs in postChoice0.
Choice adventures 106, 123, 142, 146, 276, 612, 794, 866, 1005, 1006, 1007, 1008, 1009, 1010, 1011, 1012, 1013, 1023, 1024, 1028, 1085 and 1086 have handling in preChoice that belongs in postChoice1.
Choice adventures 9, 10, 11, 12, 1171 and 1345 have handling in preChoice that belongs in postChoice2.
 

lostcalpolydude

Developer
Staff member
I expect that queue tracking was meant to be code that didn't require everything to be coded explicitly. I can certainly see that tracking being improved if there is knowledge about whether every option from every choice takes a turn. Along the way, turns_spent tracking can probably have (some of?) its edge cases fixed, since charpane/api results would no longer be required.

On the other hand, there would still need to be code to handle unknown choices from new content.

Using an item that enters a choice should set your location to none, which should prevent the choice from being counted for any location. I haven't looked at that code in a while, and maybe newer choice-causing items haven't been added to that code.
 

fronobulax

Developer
Staff member
More top of my head comments.

I think there is a requirement that when KoLmafia encounters a choice adventure it has never seen it should first do not harm and then, if possible, do something reasonable if that is obvious. If it can go a step further and record data so the next encounter is not considered new or unseen that would be good. If it could emit text or files that could be given to a dev to merge into existing files so the choice is known for everyone after commit and build (or equivalent) that would be great. The obvious example of this is data files that can have local overrides.

Whether we can create a generic choice adventure thingie, populate it with data from a file and then use it as a specific choice adventure is not clear to me yet. I am also not clear what fraction of choice adventures could be represented in this way. I don't expect a file to support choices that have situationally dependent responses given that I would rather the complexity b in the code and not in the file format.

Is the location of item triggered choices a code issue or a missing or incorrect data issue?

As a dev I am unlikely to look at code to determine whether a choice consumes a turn or not so I am not sure how having that info is beneficial. I am more likely to use experience, the wiki or just assume it doesn't until informed otherwise :)
 

fredg1

Member
I think there is a requirement that when KoLmafia encounters a choice adventure it has never seen it should first do not harm and then, if possible, do something reasonable if that is obvious. If it can go a step further and record data so the next encounter is not considered new or unseen that would be good. If it could emit text or files that could be given to a dev to merge into existing files so the choice is known for everyone after commit and build (or equivalent) that would be great. The obvious example of this is data files that can have local overrides.
Gausie has his "Excavator" spading script. A hook could be added to it that spades unknown choices and options.

If the choice adventures information is code-driven, an unknown choice/option would be seen as such at best once per session.
If the information is data/text-driven, unknown choices/options would be seen as such at best once per person, period.
This would be a point in favour of going data-driven.
Whether we can create a generic choice adventure thingie, populate it with data from a file and then use it as a specific choice adventure is not clear to me yet. I am also not clear what fraction of choice adventures could be represented in this way. I don't expect a file to support choices that have situationally dependent responses given that I would rather the complexity b in the code and not in the file format.
This was also my main concern when the suggestion of having it all be data-driven was raised.
Later on, it was established that only the "default" state of each choice adventure would go there, but even then, not only was I still questioning the feasibility of such thing, which was still not addressed, this would also mean a subsequent issue in which the location of the default state of a choice adventure (the text file) is different/far from the location of where this state is modified (the code), making it hard to tell if you're making a mistake.
Is the location of item triggered choices a code issue or a missing or incorrect data issue?
main.php?comb=1, using the lock picking skill, using the "seek a bird" skill your 7th time, using the force in combat... there's a lot of "item-based" noncombats that don't start with a request to inv_use.php.
The answer would really be to note in which location the NCs happen.
As a dev I am unlikely to look at code to determine whether a choice consumes a turn or not so I am not sure how having that info is beneficial. I am more likely to use experience, the wiki or just assume it doesn't until informed otherwise :)
The default is definitely that it doesn't until proven otherwise.
A choice adventure ('s option) that costs a turn when we thought it didn't won't break anything. It will only cause some minor de-sync with features relying on it (which there is currently none, obviously. There is one incoming feature waiting for this kind of information to be added, though).

To be honest, that very information (whether a choice adventure ('s option) costs a turn) is what initially drove me to do this refactor. However, at this point, it's not like I'd "do anything to keep it in".
 

lostcalpolydude

Developer
Staff member
main.php?comb=1, using the lock picking skill, using the "seek a bird" skill your 7th time, using the force in combat... there's a lot of "item-based" noncombats that don't start with a request to inv_use.php.
That's why, for better or worse, the code currently exists in GenericRequest. So perhaps it could be handled better, but the current code structure allows for handling all of those things, and all of those listed cases are from newer content that hasn't been added.
 

fredg1

Member
Let's start with the simple goal of helping with the organization of ChoiceManager.java.
Currently, it is very hard to tell in which of the "main" methods you are in (between CHOICE_DATA's initialization, dynamicChoiceSpoilers, dynamicChoiceOptions, preChoice, postChoice0, postChoice1, postChoice2, visitChoice, specialChoiceDecision2 and registerRequest )
and even harder to tell what handling is done for a given choice adventure.

The current goal would be to the methods by choice adventure, rather than having one big method with a long switch statement.
The benefits:
  • It would make it easier to tell if some logic is in the wrong method, as it would now be easy to see all the handling related to a given choice adventure, as well as making it easier to find the methods' JavaDocs (which are currently absent, but are planed).
  • By moving the choice adventure-specific logic away from ChoiceManager, it would help clear it up.
    Currently, such logic is clogging up ChoiceManager, making it difficult to study the logic behind choice adventure handling, making it hard to tell what a method is for (exacerbating the previous problem, as it would result in more handling going in the wrong methods).
Here is the first patch of this, which exclusively touched postChoice2 (both as a "draft" of some sort, as well as a "first step")

Any objection/concerns?
 

Attachments

  • CADatabase_postChoice2.patch
    337.1 KB · Views: 4

fredg1

Member
Bump. This seems to have been mostly ignored in the past two months. In the meantime, suggestions went from patches to git PRs.

I can turn this into a PR, but knowing if there is any objection/concern to the concept would still be appreciated.

Should I also still go method-by-method? (i.e. a PR dedicated to postChoice2, then, once/if accepted, a PR dedicated to another method, then, once/if...)
Should I do all of them at once? (doing one method per commit, and listing any modification made to *anything* in the commit message)
 

heeheehee

Developer
Staff member
My first reaction is that I don't want to read through a 10k-line patch, so I'll skim it at most.

My second reaction is that I believe there's general consensus that ChoiceManager is intractable to work with, and arranging things by adventures instead of by when the handling happens is most likely a good thing.

My third reaction is that arranging this code in classes like "abstract class CADatabase1000to1099" feels wrong, as it requires knowing all the relevant choice numbers (or being able to find it via grep) in order to know how to fix some logic. It introduces awkwardness where if choice adventure numbers for a piece of content cover xx99 - xy00, then I have to look in two separate files. And, there's no enforcement for these files being limited to 100 choices. It's ultimately lumping in a bunch of unrelated functionality (which admittedly isn't worse than the status quo, but it's not really better, either).

My fourth reaction is that there's no way I'm merging this without very, very extensive tests to make sure that we're not breaking any behavior.

If you want to get the ball rolling here, I'd suggest starting with a PR for ChoiceAdventureDatabase plus one choice adventure (not 350), plus tests. I see there's a single test for "duplicates is non-empty" but that doesn't cover any mechanics of dispatch, or choice handling is done, or really anything else.
 

fredg1

Member
arranging this code in classes like "abstract class CADatabase1000to1099" feels wrong, as it requires knowing all the relevant choice numbers (or being able to find it via grep) in order to know how to fix some logic.
Isn't it already how things are right now? ChoiceManager is 20k lines long, so you're going to have to grep (or any other text-finding tool) either way, afaik.
And, there's no enforcement for these files being limited to 100 choices.
This could be... an interesting challenge.
Would it help if I were to manage to enforce this?
 

fronobulax

Developer
Staff member
Isn't it already how things are right now? ChoiceManager is 20k lines long, so you're going to have to grep (or any other text-finding tool) either way, afaik.

This could be... an interesting challenge.
Would it help if I were to manage to enforce this?

My intuition says it is the wrong thing to enforce.

In my list of places where the code could be better by 2021 standards, this is not high on my list although I am not as opposed as I once was.

My Program Manger side of things is saddened by the fact that the language server has PRs languishing and yet some other project is resurrected.
 

heeheehee

Developer
Staff member
Isn't it already how things are right now? ChoiceManager is 20k lines long, so you're going to have to grep (or any other text-finding tool) either way, afaik.
Yes and no. When I say you'd need grep, I mean you'd need to search across multiple files to even figure out which one to edit. This gets worse if you want to edit multiple things that behave similarly but were released over a longer stretch of time, like Sea content, or Elemental airports.

If we're going through the effort to refactor the code, I would like to make sure we get it right the first time (as opposed to needing another 10k-line refactor down the line).

This could be... an interesting challenge.
Would it help if I were to manage to enforce this?
No. It's an arbitrary convention. I was pointing out the lack of enforcement as yet another reason why I don't like that approach, not the primary reason by any means.
 

fredg1

Member
Yes and no. When I say you'd need grep, I mean you'd need to search across multiple files to even figure out which one to edit. This gets worse if you want to edit multiple things that behave similarly but were released over a longer stretch of time, like Sea content, or Elemental airports.

If we're going through the effort to refactor the code, I would like to make sure we get it right the first time (as opposed to needing another 10k-line refactor down the line).


No. It's an arbitrary convention. I was pointing out the lack of enforcement as yet another reason why I don't like that approach, not the primary reason by any means.
So... should it really just be one giant file..?
 
Top