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

FRLG+ whiteout message #4967

Merged
merged 8 commits into from
Aug 2, 2024
Merged

Conversation

cawtds
Copy link

@cawtds cawtds commented Jul 13, 2024

Description

Adds FRLG type whiteout sequence with message screen and event script.

Images

example

Issue(s) that this PR fixes

Fixes #4082

Discord contact info

.cawt

Copy link
Collaborator

@Bassoonian Bassoonian left a comment

Choose a reason for hiding this comment

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

Some are actual necessary changes, some are just some ideas on how we could potentially clean it up. I'm very excited as you know, so I'd rather have some other reviewer also chime in to make sure everything is as it should be :)

@@ -8,6 +8,7 @@
#define OW_POISON_DAMAGE GEN_LATEST // In Gen4, Pokémon no longer faint from Poison in the overworld. In Gen5+, they no longer take damage at all.
#define OW_DOUBLE_APPROACH_WITH_ONE_MON FALSE // If enabled, you can be spotted by two trainers at the same time even if you only have one eligible Pokémon in your party.
#define OW_HIDE_REPEAT_MAP_POPUP FALSE // If enabled, map popups will not appear if entering a map with the same Map Section Id as the last.
#define OW_FRLG_WHITEOUT TRUE // If enabled, shows an additional whiteout message and post whiteout event script with healing NPC
Copy link
Collaborator

Choose a reason for hiding this comment

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

Config should be off by default

Suggested change
#define OW_FRLG_WHITEOUT TRUE // If enabled, shows an additional whiteout message and post whiteout event script with healing NPC
#define OW_FRLG_WHITEOUT FALSE // If enabled, shows an additional whiteout message and post whiteout event script with healing NPC

&& warpData->y == loc->y;
}

static void SetWhiteoutRespawnHealerNpcAsLastTalked(u32 healLocationId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static void SetWhiteoutRespawnHealerNpcAsLastTalked(u32 healLocationId)
static void SetWhiteoutRespawnHealerNPCAsLastTalked(u32 healLocationId)

gSpecialVar_LastTalked = sWhiteoutRespawnHealerNpcLocalIds[healLocationId - 1];
}

void SetWhiteoutRespawnWarpAndHealerNpc(struct WarpData * warp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void SetWhiteoutRespawnWarpAndHealerNpc(struct WarpData * warp)
void SetWhiteoutRespawnWarpAndHealerNPC(struct WarpData * warp)

@@ -24,5 +24,6 @@
#define HEAL_LOCATION_EVER_GRANDE_CITY_POKEMON_LEAGUE 20
#define HEAL_LOCATION_SOUTHERN_ISLAND_EXTERIOR 21
#define HEAL_LOCATION_BATTLE_FRONTIER_OUTSIDE_EAST 22
#define HEAL_LOCATION_MAX (HEAL_LOCATION_BATTLE_FRONTIER_OUTSIDE_EAST)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional, but I think we generally would use HEAL_LOCATION_COUNT with a +1 here

Comment on lines 1282 to 1283


Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

warp->mapNum = sHealLocationsPokemonCenter[healLocationId - 1][1];
warp->warpId = 0xFF;

if (sHealLocationsPokemonCenter[healLocationId - 1][0] == MAP_GROUP(LITTLEROOT_TOWN_BRENDANS_HOUSE_1F) && sHealLocationsPokemonCenter[healLocationId - 1][1] == MAP_NUM(LITTLEROOT_TOWN_BRENDANS_HOUSE_1F))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we can never both have FRLG whiteouts and the normal ones, I wonder if these hardcoded coordinates should instead become part of a config-dependent sHealLocations. Just an idea

Copy link
Author

Choose a reason for hiding this comment

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

HEAL_LOCATION_SOUTHERN_ISLAND_EXTERIOR uses the normal whiteout even with FRLG enabled. I think it might also be possible to replace struct WarpData lastHealLocation in SaveBlock1 with just a u8 healLocationId saving 4 byte. This would also make checking for the location much easier. Though this would obviously be save breaking.

Choose a reason for hiding this comment

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

Does HEAL_LOCATION_SOUTHERN_ISLAND_EXTERIOR exist in ORAS? What does it do there?

Copy link
Author

@cawtds cawtds Jul 14, 2024

Choose a reason for hiding this comment

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

Not sure, but it is only used as a flight destination and is never set, since it is not possible to target Southern Island as a destination. I don't think it was ever intended as a heal location, just planned as a possible flight path. HEAL_LOCATION is imo a bit of a misleading name, since Teleport and Fly also use them.

src/data/heal_locations_pkm_center.h Show resolved Hide resolved

switch (gTasks[taskId].tState)
{
case 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we please replace the case numbers with constants? It would just be easier to understand what's happening ,especially because case 4 is after 2 and 5 lmao

CopyWindowToVram(windowId, COPYWIN_FULL);

// Scene changes if last heal location was the player's house
if (IsLastHealLocation(HEAL_LOCATION_LITTLEROOT_TOWN_MAYS_HOUSE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love this giant if statement. Is it possible to add them to a whitelist of "home" locations and then change IsLastHealLocation to GetLastHealLocation and check that against the list? IsRegularLandTrainer is an example

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay now that I've looked at IsLastHealLocation I didn't realize you were checking 5 different conditions lmao, probably not possible

{
if (healLocationId == HEAL_LOCATION_NONE || healLocationId >= HEAL_LOCATION_COUNT)
return FALSE;
return sWhiteoutRespawnHealerNpcLocalIds[healLocationId - 1] > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the expected behavior when no heal location is set?

When I whiteout without ever setting a heal location, I get sent to Battle Frontier East and I heal outside lmao.

Copy link
Author

Choose a reason for hiding this comment

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

Missing a bit of context here. What exactly isn't set? sHealLocation, sHealLocationsPokemonCenter, sWhiteoutRespawnHealerNpcLocalIds?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sHealLocation. If you use cheat start and a script, and whiteout inside the truck, you'll see the behavior.

That's also what I was referring to with #4967 (comment)

seen here: https://youtu.be/NhYclVXCmN4?t=499

src/data/heal_locations_pkm_center.h Show resolved Hide resolved
Copy link
Collaborator

@pkmnsnfrn pkmnsnfrn left a comment

Choose a reason for hiding this comment

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

Works perfectly in game. Consider all of my feedback optional, just resolve / commit the suggestions and tag me when you're done and I'll merge.

src/field_screen_effect.c Outdated Show resolved Hide resolved
src/field_screen_effect.c Outdated Show resolved Hide resolved
src/field_screen_effect.c Outdated Show resolved Hide resolved
include/config/overworld.h Outdated Show resolved Hide resolved
src/data/heal_locations_pkm_center.h Outdated Show resolved Hide resolved
src/heal_location.c Outdated Show resolved Hide resolved
src/heal_location.c Outdated Show resolved Hide resolved
include/heal_location.h Outdated Show resolved Hide resolved
src/heal_location.c Outdated Show resolved Hide resolved
src/overworld.c Outdated Show resolved Hide resolved
@pkmnsnfrn pkmnsnfrn merged commit 3af93bd into rh-hideout:upcoming Aug 2, 2024
1 check passed
@cawtds cawtds deleted the frlg-whiteout-message branch August 12, 2024 16:01
@AsparagusEduardo AsparagusEduardo added category: battle-mechanic Pertains to battle mechanics new-feature Adds a feature category: overworld Pertains to out-of-battle mechanics labels Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: battle-mechanic Pertains to battle mechanics category: overworld Pertains to out-of-battle mechanics new-feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants