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.

Sign up for GitHub

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

Jump to bottom

Expose Thieves' Hideout selection for keyrings in randomizer settings #2890

Merged
merged 5 commits into from
May 26, 2023
Merged

Expose Thieves' Hideout selection for keyrings in randomizer settings #2890

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5244cda
expose thieves hideout selection for keyrings in randomizer settings
Archez May 16, 2023
8f5538a
Adds validation to GF Key Ring and Key Ring Count settings.
leggettc18 May 25, 2023
1351334
Remove magic numbers and fix formatting.
leggettc18 May 25, 2023
f6b9c22
Fixes bad Key Ring Count under certain conditions.
leggettc18 May 25, 2023
015c30d
Merge pull request #2 from leggettc18/gf-keyring
Archez May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions soh/soh/Enhancements/presets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ const std::vector<const char*> randomizerCvars = {
"gRandomizeShuffleKeyRingsFireTemple",
"gRandomizeShuffleKeyRingsForestTemple",
"gRandomizeShuffleKeyRingsGanonsCastle",
"gRandomizeShuffleKeyRingsGerudoFortress",
"gRandomizeShuffleKeyRingsGTG",
"gRandomizeShuffleKeyRingsRandomCount",
"gRandomizeShuffleKeyRingsShadowTemple",
Expand Down
26 changes: 13 additions & 13 deletions soh/soh/Enhancements/randomizer/3drando/settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ namespace Settings {
Option LACSDungeonCount = Option::U8 ("Dungeon Count", {NumOpts(0, 8)}, {lacsDungeonCountDesc}, OptionCategory::Setting, 1, true);
Option LACSTokenCount = Option::U8 ("Token Count", {NumOpts(0, 100)}, {lacsTokenCountDesc}, OptionCategory::Setting, 1, true);
Option KeyRings = Option::U8 ("Key Rings", {"Off", "Random", "Count", "Selection"}, {keyRingDesc});
Option KeyRingsRandomCount = Option::U8 ("Keyring Dungeon Count", {NumOpts(0, 8)}, {keyRingDesc}, OptionCategory::Setting, 1);
Option KeyRingsRandomCount = Option::U8 ("Keyring Dungeon Count", {NumOpts(0, 9)}, {keyRingDesc}, OptionCategory::Setting, 1);
Option RingFortress = Option::Bool("Gerudo Fortress", {"Off", "On"}, {keyRingDesc}, OptionCategory::Setting);
Option RingForest = Option::Bool("Forest Temple", {"Off", "On"}, {keyRingDesc}, OptionCategory::Setting);
Option RingFire = Option::Bool("Fire Temple", {"Off", "On"}, {keyRingDesc}, OptionCategory::Setting);
Expand Down Expand Up @@ -2155,7 +2155,8 @@ namespace Settings {
LACSTokenCount.Hide();
}

if (KeyRings.IsNot(KEYRINGS_OFF)) {
// Only show dungeons when keyring selection is enabled
if (KeyRings.Is(KEYRINGS_SELECTION)) {
for (Option *option : keyRingOptions) {
option->Unhide();
}
Expand Down Expand Up @@ -2829,6 +2830,7 @@ namespace Settings {

KeyRings.SetSelectedIndex(cvarSettings[RSK_KEYRINGS]);
KeyRingsRandomCount.SetSelectedIndex(cvarSettings[RSK_KEYRINGS_RANDOM_COUNT]);
RingFortress.SetSelectedIndex(cvarSettings[RSK_KEYRINGS_GERUDO_FORTRESS]);
RingForest.SetSelectedIndex(cvarSettings[RSK_KEYRINGS_FOREST_TEMPLE]);
RingFire.SetSelectedIndex(cvarSettings[RSK_KEYRINGS_FIRE_TEMPLE]);
RingWater.SetSelectedIndex(cvarSettings[RSK_KEYRINGS_WATER_TEMPLE]);
Expand Down Expand Up @@ -2923,22 +2925,20 @@ namespace Settings {
}
}

std::vector<uint8_t> randKeyRingDungeons = {};
//Set key ring for each dungeon
for (size_t i = 0; i < dungeons.size(); i++) {
dungeons[i]->ClearKeyRing();
if (dungeons[i]->GetSmallKeyCount() > 0) {
randKeyRingDungeons.push_back(i);
}
}
if (KeyRings.Is(KEYRINGS_RANDOM) || KeyRings.Is(KEYRINGS_RANDOM_COUNT)) {
int keyRingCount = KeyRings.Is(KEYRINGS_RANDOM_COUNT) ? KeyRingsRandomCount.Value<uint8_t>() : Random(0, randKeyRingDungeons.size());
Shuffle(randKeyRingDungeons);

for (uint8_t i = 0; i < keyRingCount; i++) {
dungeons[randKeyRingDungeons[i]]->SetKeyRing();
if (KeyRings) {
// Random Key Rings
if (KeyRings.Is(KEYRINGS_RANDOM) || KeyRings.Is(KEYRINGS_RANDOM_COUNT)) {
auto keyRings = keyRingOptions;
int keyRingCount = KeyRings.Is(KEYRINGS_RANDOM_COUNT) ? KeyRingsRandomCount.Value<uint8_t>() : Random(0, keyRings.size());
Shuffle(keyRings);
for (size_t i = 0; i < keyRingCount; i++) {
keyRings[i]->SetSelectedIndex(ON);
}
}
} else if (KeyRings.Is(KEYRINGS_SELECTION)) {
if (RingWell) {
BottomOfTheWell.SetKeyRing();
}
Expand Down
4 changes: 2 additions & 2 deletions soh/soh/Enhancements/randomizer/draw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ extern "C" void Randomizer_DrawSmallKey(PlayState* play, GetItemEntry* getItemEn
{ 126, 16, 177 }, // Shadow Temple
{ 227, 110, 255 }, // Bottom of the Well
{ 221, 212, 60 }, // Gerudo Training Grounds
{ 255, 255, 255 }, // Theive's Hideout (unused)
{ 255, 255, 255 }, // Thieves' Hideout
{ 80, 80, 80 } // Ganon's Castle
};

Expand Down Expand Up @@ -110,7 +110,7 @@ extern "C" void Randomizer_DrawKeyRing(PlayState* play, GetItemEntry* getItemEnt
{ 126, 16, 177 }, // Shadow Temple
{ 227, 110, 255 }, // Bottom of the Well
{ 221, 212, 60 }, // Gerudo Training Grounds
{ 255, 255, 255 }, // Theive's Hideout (unused)
{ 255, 255, 255 }, // Thieves' Hideout
{ 80, 80, 80 } // Ganon's Castle
};

Expand Down
15 changes: 12 additions & 3 deletions soh/soh/Enhancements/randomizer/randomizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ std::unordered_map<std::string, RandomizerSettingKey> SpoilerfileSettingNameToEn
{ "Shuffle Dungeon Items:Token Count", RSK_LACS_TOKEN_COUNT },
{ "Shuffle Dungeon Items:Key Rings", RSK_KEYRINGS },
{ "Shuffle Dungeon Items:Keyring Dungeon Count", RSK_KEYRINGS_RANDOM_COUNT },
{ "Shuffle Dungeon Items:Gerudo Fortress", RSK_KEYRINGS_GERUDO_FORTRESS },
{ "Shuffle Dungeon Items:Forest Temple", RSK_KEYRINGS_FOREST_TEMPLE },
{ "Shuffle Dungeon Items:Fire Temple", RSK_KEYRINGS_FIRE_TEMPLE },
{ "Shuffle Dungeon Items:Water Temple", RSK_KEYRINGS_WATER_TEMPLE },
Expand Down Expand Up @@ -845,6 +846,7 @@ void Randomizer::ParseRandomizerSettingsFile(const char* spoilerFileName) {
case RSK_KAK_50_SKULLS_HINT:
case RSK_WARP_SONG_HINTS:
case RSK_SCRUB_TEXT_HINT:
case RSK_KEYRINGS_GERUDO_FORTRESS:
case RSK_KEYRINGS_FOREST_TEMPLE:
case RSK_KEYRINGS_FIRE_TEMPLE:
case RSK_KEYRINGS_WATER_TEMPLE:
Expand Down Expand Up @@ -2966,6 +2968,7 @@ void GenerateRandomizerImgui(std::string seed = "") {
cvarSettings[RSK_GERUDO_KEYS] = CVarGetInteger("gRandomizeGerudoKeys", RO_GERUDO_KEYS_VANILLA);
cvarSettings[RSK_KEYRINGS] = CVarGetInteger("gRandomizeShuffleKeyRings", RO_KEYRINGS_OFF);
cvarSettings[RSK_KEYRINGS_RANDOM_COUNT] = CVarGetInteger("gRandomizeShuffleKeyRingsRandomCount", 8);
cvarSettings[RSK_KEYRINGS_GERUDO_FORTRESS] = CVarGetInteger("gRandomizeShuffleKeyRingsGerudoFortress", 0);
cvarSettings[RSK_KEYRINGS_FOREST_TEMPLE] = CVarGetInteger("gRandomizeShuffleKeyRingsForestTemple", 0);
cvarSettings[RSK_KEYRINGS_FIRE_TEMPLE] = CVarGetInteger("gRandomizeShuffleKeyRingsFireTemple", 0);
cvarSettings[RSK_KEYRINGS_WATER_TEMPLE] = CVarGetInteger("gRandomizeShuffleKeyRingsWaterTemple", 0);
Expand Down Expand Up @@ -3921,20 +3924,26 @@ void DrawRandoEditor(bool& open) {
"\n"
"Off - No dungeons will have their keys replaced with keyrings.\n"
"\n"
"Random - A random amount of dungeons(0-8) will have their keys replaced with keyrings.\n"
"Random - A random amount of dungeons(0-9) will have their keys replaced with keyrings.\n"
"\n"
"Count - A specified amount of randomly selected dungeons will have their keys replaced with keyrings.\n"
"\n"
"Selection - Hand select which dungeons will have their keys replaced with keyrings."
"Selection - Hand select which dungeons will have their keys replaced with keyrings.\n"
"\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 "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Can ImGui handle the max number of the slider changing at runtime

IIRC yes

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the PR, assuming this will resolve our concerns related to the wording now 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Provided everyone else is fine with how I did it, then yes.

"locations or Gerudo's Fortress is set to Fast (Rescue One Carpenter)."
);
UIWidgets::EnhancementCombobox("gRandomizeShuffleKeyRings", randoShuffleKeyRings, RO_KEYRINGS_OFF);
ImGui::PopItemWidth();
switch (CVarGetInteger("gRandomizeShuffleKeyRings", RO_KEYRINGS_OFF)) {
case RO_KEYRINGS_COUNT:
UIWidgets::PaddedEnhancementSliderInt("Key Ring Count: %d", "##RandomizeShuffleKeyRingsRandomCount",
"gRandomizeShuffleKeyRingsRandomCount", 1, 8, "", 8, true, true, false);
"gRandomizeShuffleKeyRingsRandomCount", 1, 9, "", 9, true, true, false);
break;
case RO_KEYRINGS_SELECTION:
UIWidgets::EnhancementCheckbox("Gerudo Fortress##RandomizeShuffleKeyRings", "gRandomizeShuffleKeyRingsGerudoFortress");
UIWidgets::EnhancementCheckbox("Forest Temple##RandomizeShuffleKeyRings", "gRandomizeShuffleKeyRingsForestTemple");
UIWidgets::EnhancementCheckbox("Fire Temple##RandomizeShuffleKeyRings", "gRandomizeShuffleKeyRingsFireTemple");
UIWidgets::EnhancementCheckbox("Water Temple##RandomizeShuffleKeyRings", "gRandomizeShuffleKeyRingsWaterTemple");
Expand Down
1 change: 1 addition & 0 deletions soh/soh/Enhancements/randomizer/randomizerTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,6 +1072,7 @@ typedef enum {
RSK_LACS_TOKEN_COUNT,
RSK_KEYRINGS,
RSK_KEYRINGS_RANDOM_COUNT,
RSK_KEYRINGS_GERUDO_FORTRESS,
RSK_KEYRINGS_FOREST_TEMPLE,
RSK_KEYRINGS_FIRE_TEMPLE,
RSK_KEYRINGS_WATER_TEMPLE,
Expand Down