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

Create enum classes for EXI devices and slots #10364

Merged
merged 7 commits into from Jan 16, 2022

Conversation

Pokechu22
Copy link
Contributor

@Pokechu22 Pokechu22 commented Jan 11, 2022

This is something I started on a while back while working on redoing both the gamecube boot process (related to the broken AD16 implementation) and implementing support for SD geckos/the official SD adapter/SD2SP2; I haven't finished either of those, but this is a source of merge conflicts as well as a general improvement, so I might as well PR it. This PR is refactoring with no expected behavior differences, but I haven't done extensive testing on it.

@Pokechu22 Pokechu22 force-pushed the exi-device-refactor branch 2 times, most recently from 053c60a to eb3e188 Compare January 11, 2022 21:37
else
memorycard_device = EXIDEVICE_MEMORYCARD;
memorycard_device = EXIDeviceType::MemoryCard;
Copy link

@Warepire Warepire Jan 11, 2022

Choose a reason for hiding this comment

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

Can't you fold this like on line 61? And then the entire if block collapses into if Not IsUsingMemcard, set None, otherwise use the configured value? Or did I miss something here, logically?

Copy link
Contributor Author

@Pokechu22 Pokechu22 Jan 11, 2022

Choose a reason for hiding this comment

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

I'm not sure. I think the purpose here is to force it to be a memory card (and theoretically directly using the configured value could result in something other than a memory card); the Movie/TAS code only supports indicating whether a memory card is in use, and doesn't support other devices. But if we're overriding settings for movie purposes, why bother supporting both raw memory cards and memory card folders? Someone more experienced with the movie functions would have to chime in here.

To be clear, I'm assuming you're suggesting something like this:

void AddMemoryCards(Slot slot)
{
  EXIDeviceType memorycard_device;
  if (Movie::IsPlayingInput() && Movie::IsConfigSaved())
  {
    if (Movie::IsUsingMemcard(slot))
      memorycard_device = Config::Get(Config::GetInfoForEXIDevice(slot));
      // or:
      // memorycard_device = EXIDeviceType::MemoryCard;
    else
      memorycard_device = EXIDeviceType::None;
  }
  else
  {
    memorycard_device = Config::Get(Config::GetInfoForEXIDevice(slot));
  }

  g_Channels[SlotToEXIChannel(slot)]->AddDevice(memorycard_device, SlotToEXIDevice(slot));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it further, I think the best approach would be to get rid of AddMemoryCards entirely and then handle this in MovieConfigLoader which is injected into the config system (CC @AdmiralCurtiss). That wasn't possible until recently when the EXI and other settings were moved to the new config system.

JosJuice says that there is a benefit to supporting both raw memory cards and memory card folders, since users need to manually supply the save file (it's not included in the move file). On the other hand, devices other than memory cards aren't supported and won't sync at all (at least for things like the microphone). So forcing it to be a memory card works, but I've changed it to instead stick with the user-specified device and generate a panic alert if an invalid device is selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would agree, handling this via LoadFromDTM() in MovieConfigLoader.cpp is likely the best approach here.

return device;
}
return nullptr;
return g_Channels.at(SlotToEXIChannel(slot))->GetDevice(1 << SlotToEXIDevice(slot));

Choose a reason for hiding this comment

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

What happens if slot for some reason is out of range? Or is slot always valid here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SlotToEXIChannel and SlotToEXIDevice will trigger a PanicAlertFmt if slot is invalid and return 0. Those functions will always return something within the valid device and channel ranges.

Source/Core/Core/HW/EXI/EXI_Device.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/EXI/EXI.cpp Outdated Show resolved Hide resolved
Source/Core/Core/HW/EXI/EXI_Device.h Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated Show resolved Hide resolved
Source/Core/DolphinQt/Settings/GameCubePane.cpp Outdated Show resolved Hide resolved
Source/Core/Core/Config/MainSettings.cpp Show resolved Hide resolved
This simplifies the code in GameCubePane, and allows us to use the EXIDeviceType enum in error messages.
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss left a comment

Choose a reason for hiding this comment

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

Largely untested (I booted up a game and switched memory cards, and that worked, but not more than that) but code looks good to me now.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

Code LGTM, untested.

for (Slot slot : MEMCARD_SLOTS)
AddMemoryCards(slot);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a preexisting issue but would it make sense to move the loop inside AddMemoryCards? The function is only called from here, and as it stands it adds a single device despite the name implying otherwise.

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 think that somewhat makes sense, but I'd rather rip out the entirety of AddMemoryCards and replace it with the new config system in a separate PR. I can rename the function to AddMemoryCard pretty easily though.

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • DKCR-fast-depth on mvk-osx-m1: diff
  • ea-pink on mvk-osx-m1: diff
  • rs2-skybox on mvk-osx-m1: diff
  • rs3-bumpmapping on mvk-osx-m1: diff

automated-fifoci-reporter

@Pokechu22 Pokechu22 merged commit b7ac110 into dolphin-emu:master Jan 16, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants