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

Rando: More Misc. Hints #2930

Merged
merged 33 commits into from
Sep 19, 2023

Conversation

stratomaster64
Copy link
Contributor

@stratomaster64 stratomaster64 commented May 31, 2023

Adds:

  • Saria as an NPC to receive a hint on where the progressive magic meter would be.
    • The ability to utilize Saria's song to get this hint if you don't see her at her usual spots.
  • Hint for frog ocarina game by stepping on the platform near them in Zora's River.
  • Sheik as an NPC to receive the light arrows hint, when trials are required.
    • You can see her in the Temple of Time and Ganon's Castle as adult.
    • Ganondorf will only say the funny line, but this should allow light arrow hint to only need the rainbow bridge.
  • A function within randomizer.cpp to allow for context-sensitive message replacement...I'm guessing? It just ended up that way.
    Hopefully we should hit hint parity once this makes it in.

Build Artifacts

@stratomaster64 stratomaster64 marked this pull request as ready for review May 31, 2023 18:14
@leggettc18 leggettc18 added this to the MacReady milestone Jun 21, 2023
@stratomaster64
Copy link
Contributor Author

May want to make this PR dependent on #2981 just so I can handle Sheik's MS hint text all in one go, rather than make a follow-up PR.

Copy link
Contributor

@aMannus aMannus left a comment

Choose a reason for hiding this comment

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

Initial quick scan from me. Looks like it follows the same pattern as many other existing hints, so that's great. I do have some questions.

soh/soh/Enhancements/randomizer/3drando/hints.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/randomizer/randomizer.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/randomizer/randomizer.cpp Outdated Show resolved Hide resolved
@Malkierian
Copy link
Contributor

Other than what's been mentioned already by others, the code looks good to me. If I get around to actually playtesting it, I'll drop an approving review.

Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

A few comments, solid wooooork

@@ -5229,6 +5274,70 @@ CustomMessage Randomizer::GetWarpSongMessage(u16 textId, bool mysterious) {
return messageEntry;
}

CustomMessage Randomizer::GetMiscMessage(s16 scene, u16 originalTextId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see much benefit to this being one function that splits into 3 blocks vs just 3 different functions personally. And the scene argument only seems relevant for the middle block

That being said, I think a lot of these custom message handlers could be cleaned up so up to you if you want to make the switch, I think this system will look a bit different for v3/v4 ™️

Copy link
Contributor

Choose a reason for hiding this comment

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

i also think splitting it into 3 makes sense
GetFrogMessage
GetSheikMessage
GetSariaMessage

we're already making calls for each from separate places in CustomMessage_RetrieveIfExists, so we might as well call into different methods.

it could be cool to see a bunch of the logic in CustomMessage_RetrieveIfExists moved out and cleaned up, but that feels like it should be its own PR

soh/soh/Enhancements/randomizer/randomizer.cpp Outdated Show resolved Hide resolved
soh/soh/SaveManager.cpp Outdated Show resolved Hide resolved
soh/src/code/z_play.c Outdated Show resolved Hide resolved
stratomaster64 and others added 4 commits September 14, 2023 12:46
Sheik no longer cheats with room transitions;
Enforced text IDs on saria msg function
Sheik no longer cheats with room transitions;
Enforced text IDs on saria msg function
Co-authored-by: Garrett Cox <garrettjcox@gmail.com>
Copy link
Contributor

@garrettjoecox garrettjoecox left a comment

Choose a reason for hiding this comment

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

Noice.

soh/soh/Enhancements/mods.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

overall this is looking really good! couple little comments in there but nothing major.

void RegisterRandomizerSheikSpawn() {
GameInteractor::Instance->RegisterGameHook<GameInteractor::OnSceneSpawnActors>([]() {
if (!gPlayState) return;
bool canSheik = OTRGlobals::Instance->gRandomizer->GetRandoSettingValue(RSK_TRIAL_COUNT) != RO_GANONS_TRIALS_SKIP;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to spawn sheik even when the light arrow hint is turned off? i think i'd vote to have sheik only appear when the setting is on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be a combination of the two, as Sheik relies on LA hint and there being trials (otherwise you go to Ganondorf for LA hint).

@@ -5229,6 +5274,70 @@ CustomMessage Randomizer::GetWarpSongMessage(u16 textId, bool mysterious) {
return messageEntry;
}

CustomMessage Randomizer::GetMiscMessage(s16 scene, u16 originalTextId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i also think splitting it into 3 makes sense
GetFrogMessage
GetSheikMessage
GetSariaMessage

we're already making calls for each from separate places in CustomMessage_RetrieveIfExists, so we might as well call into different methods.

it could be cool to see a bunch of the logic in CustomMessage_RetrieveIfExists moved out and cleaned up, but that feels like it should be its own PR

Comment on lines 2044 to 2045
if (INV_CONTENT(ITEM_ARROW_LIGHT) == ITEM_ARROW_LIGHT || !Randomizer_GetSettingValue(RSK_LIGHT_ARROWS_HINT) ||
Randomizer_GetSettingValue(RSK_LIGHT_ARROWS_HINT) && Randomizer_GetSettingValue(RSK_GANONS_TRIALS)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need this change. the point of light arrow hint outside of trials is people need the lights to get past the trials. it would be out of logic to get to dorf without them, and i guess i see no reason to not give the light arrow hint to someone who decides to trial skip past the sheik hint without lights.

this->actor.flags |= ACTOR_FLAG_TARGETABLE | ACTOR_FLAG_FRIENDLY;
if (gSaveContext.n64ddFlag && gPlayState->sceneNum == SCENE_TEMPLE_OF_TIME) {
if (!CHECK_DUNGEON_ITEM(DUNGEON_KEY_BOSS, SCENE_GANONS_TOWER)) {
this->actor.textId = 0x700F;
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be nice to use #defines or an enum for these instead of having them as magic numbers

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 agree, although this file had the original text IDs as magic numbers.

@@ -2401,6 +2432,9 @@ void EnXc_Init(Actor* thisx, PlayState* play) {
case SHEIK_TYPE_0:
EnXc_DoNothing(this, play);
break;
case -1: //Special rando case
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this not a magic number

@@ -10,6 +10,7 @@ typedef void (*EnXcActionFunc)(struct EnXc*, PlayState*);
typedef void (*EnXcDrawFunc)(struct Actor*, PlayState*);

typedef enum {
/*-1 */ SHEIK_TYPE_RANDO = -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this really makes any real difference, but typically when I've added to source enums I'll add it to the end. Not sure if one is preferable over the other

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 only really placed it there because of the number ordering.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reasoning behind using -1 instead of 10 here? just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird that I have to do it from a specific tab in GH (thanks Microsoft), but see the comment below.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible for the type to be something not in the enum with vanilla play but not negative?

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 don't think so, but my reasoning was more along the lines of "this is intentionally meant for randomizer." Either -1 or 10 wouldn't matter anyway, as Sheik will kill her actor if the rando enum is used outside randomizer anyway.

CustomMessage Randomizer::GetSariaMessage(u16 originalTextId) {
if (originalTextId == TEXT_SARIA_SFM || originalTextId == TEXT_SARIAS_SONG_FOREST_SOUNDS || TEXT_SARIAS_SONG_FOREST_TEMPLE) {
CustomMessage messageEntry = CustomMessageManager::Instance->RetrieveMessage(Randomizer::hintMessageTableID, TEXT_SARIAS_SONG_FACE_TO_FACE);
CustomMessage messageEntry2 = messageEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you instantiate a second messageEntry here? Why not operate on the first one in the rest of this function?

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 remember having issues replacing the control character with the first custom message...maybe that's not the case anymore?

Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

there are still a couple magic numbers in there (specifically in saria) but that can be a part of a bigger magic number cleanup effort later (and not part of this PR)

couple tiny naming/formatting nits but nothing blocking. i say we :shipit:

soh/soh/Enhancements/randomizer/3drando/hints.cpp Outdated Show resolved Hide resolved
soh/soh/Enhancements/randomizer/3drando/hints.hpp Outdated Show resolved Hide resolved
soh/src/code/z_play.c Outdated Show resolved Hide resolved
@@ -10,6 +10,7 @@ typedef void (*EnXcActionFunc)(struct EnXc*, PlayState*);
typedef void (*EnXcDrawFunc)(struct Actor*, PlayState*);

typedef enum {
/*-1 */ SHEIK_TYPE_RANDO = -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reasoning behind using -1 instead of 10 here? just curious

@stratomaster64
Copy link
Contributor Author

@briaguya-ai For some reason I'm not able to add to or reply to the comment regarding the rando type on Sheik. The theory was that the specific would never occur in gameplay unless intentionally set.

stratomaster64 and others added 4 commits September 16, 2023 17:00
Co-authored-by: briaguya <70942617+briaguya-ai@users.noreply.github.com>
Co-authored-by: briaguya <70942617+briaguya-ai@users.noreply.github.com>
Copy link
Contributor

@aMannus aMannus left a comment

Choose a reason for hiding this comment

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

I'd like to see the duplicate messageEntry that Malk mentioned be removed, and the -1 enum be changed, but those are both minor and not dealbreakers to me.

Nicely done 👍

@garrettjoecox garrettjoecox merged commit c3a1eb2 into HarbourMasters:develop Sep 19, 2023
8 checks passed
@stratomaster64 stratomaster64 deleted the yet-more-hints branch November 8, 2023 02:14
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.

None yet

9 participants