Expose Thieves' Hideout selection for keyrings in randomizer settings #2890
Conversation
|
/cc @garrettjoecox Not sure why Thieves' Hideout was excluded from keyrings in our code originally, but this PR aligns us with n64 rando/3ds rando regarding keyring selection/random keyrings while keeping our "count" option. |
| "\n" | ||
| "Selecting key ring for dungeons will have no effect if Small Keys are set to Start With or Vanilla.\n" | ||
| "\n" | ||
| "Selecting key ring for Thieves' Hideout will have no effect if Thieves' Hideout keys are in vanilla " |
There was a problem hiding this comment.
I think this comment here is why this didn’t go in with the rest, I don’t think I was sure how to properly handle this
There was a problem hiding this comment.
yeah, i feel like we probably want to expose this more clearly than just having a tooltip here. right now, if a player picks count 3, there will definitely be 3 keyrings in the pool. adding this into the mix makes it so there may only be 2 depending on their GF key settings.
i think to implement this we should make sure the UI communicates the setting interaction (should probably do a similar thing for start with and vanilla dungeon keys, but that feels less likely to confuse people)
There was a problem hiding this comment.
@briaguya-ai I'm not completely opposed to changing the UI, but am weary of the changes needed to track all that both in the randomizer menu, and in the 3drando code.
Before I go that approach, I'm curious if adjusting the tooltip description would be sufficient to account for any oddities. Here is one example I came up
Count - Up to a specified amount of randomly selected dungeons will have their keys replaced with keyrings. Exact count depends on key settings.
There was a problem hiding this comment.
As far as I can tell from looking at the 3drando code, Gerudo Fortress is not included in the dungeons that can be randomly selected from the count to have keyrings. The code that picks the random dungeons to give key rings to is around settings.cpp L2969 pulls from dungeonList, filters out dungeons with no keys, shuffles that list, and then gives key rings to the first keyRingCount dungeons in the shuffled list. Said dungeonList does not have Gerudo Fortress.
Meanwhile whether or not to add a Gerudo's Fortress keying to the pool is handled separately from everything else in item_pool.cpp L828-872. So whether it has a key ring or not is handled entirely independently of all the other dungeons, and it having or not having a key ring will not affect the key ring count of the other dungeons.
Therefore, the only thing to actually track for whether or not to show this option is what Gerudo Fortress access is set to. We should hide this option if it is set to either Open, Vanilla, or Fast, as those options either remove Gerudo Fortress keys altogether, or they require the Gerudo Fortress keys to be in their vanilla locations. Otherwise we show the keyring option.
There was a problem hiding this comment.
So TL;DR
@briaguya-ai, the key ring count would not be affected by the possibility of GF getting randomly selected and not being able to have a key ring, because it actually can't be randomly selected at all, see above.
@Archez The logic for hiding or showing the GF Key Ring option should be pretty simple and only dependent on GF access settings, see above.
There was a problem hiding this comment.
Can ImGui handle the max number of the slider changing at runtime? If so, I think it shouldn't be too hard to actually just decrease the max count to 8 if we get in a situation where Gerudo Fortress can't have a KeyRing (i.e. if GerudoFortress is at anything other than Normal) and also hide the checkbox for Gerudo Fortress key ring. Then in 3drando we can start with keyRingOptions not having RingFortress in it, and pushing it onto that vector before setting key rings on or off according to the keyRingCount if we can have a GerudoFortressKeyring
There was a problem hiding this comment.
Can ImGui handle the max number of the slider changing at runtime
IIRC yes
There was a problem hiding this comment.
I PR'ed a fix to the Archez's branch that adds this extra validation to disable the check box and change the max key ring count. Archez#2
There was a problem hiding this comment.
Thanks for the PR, assuming this will resolve our concerns related to the wording now 👍
There was a problem hiding this comment.
Provided everyone else is fine with how I did it, then yes.
Adds validation to GF Key Ring and Key Ring Count settings
It was found that we didn't support Thieves' Hideout keyrings in our randomizer menu. The 3drando source already supported it and we had existing RG_ item for them, you just couldn't "select" it or the random value would not select Theives' Hideout.
This PR exposes a toggle for Thieves' Hideout when keyrings is set to selection, and the count max has been increased to 9 for count mode. Random mode now also considers Thieves' Hideout as an option.
The 3ds rando setting code was tweaked to match more closely with how upstream works, but still preserving our added "count" option. This means when set to count/random, a random amount of the keyring options are set to on, and then the dungeon specific key ring options will set the dungeon to keyring mode. This way the spoiler log settings will detail which keyrings were enabled (meaning the RSK_ value is parsed correctly even when not in selection mode).
The 3ds logic will handle the correct item being used if carpenters are set to fast, or Thieves' Hideout are vanilla.
This PR is based off the spock branch, but I pointed to develop as I'm not sure if we prefer this as a "bugfix" or new feature. I can re-point the base branch if needed.
Build Artifacts