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

Gecko/AR Batch Approval #13204

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LillyJadeKatrin
Copy link
Contributor

Adding a community-requested list of Gecko and Action Replay codes to the allowlist. Many of these codes were from the wiki and are being added to Dolphin's repo for the first time.

@JMC47
Copy link
Contributor

JMC47 commented Dec 3, 2024

For something like G2V, if a setting affects all 3 regions, then it goes into G2V, then you can create the 3 region specific INIs for the region specific codes and Dolphin will construct it accordingly. You can see this in other games that have specific settings in the "global" INI and then the cheats placed into the 6 character ini.

Data/Sys/GameSettings/G4AEE9.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GAL.ini Outdated Show resolved Hide resolved
[Gecko_RetroAchievements_Verified]
$Proper 16:9 Widescreen Support [Dan Salvato, mirrorbender]
$Properly Display in 4:3 [Dan Salvato, mirrorbender]
$Normal C Stick Functionality in Singleplayer Modes [Zauron]
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want to allow normal C stick functionality in single player? Unlike the other codes you're allowlisting in this PR, this makes the game easier to play.

Copy link
Contributor

Choose a reason for hiding this comment

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

RetroAchievements does have a patch for this. Because the C-Stick is so integral to Melee at this point, it's considered a standard part of the gameplay and is core to how most people play the game nowadays.

Choose a reason for hiding this comment

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

As a Melee player myself, I see both sides of the argument. I'd think keeping original game behavior by default is more in the spirit of Dolphin though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't about defaulting it, this change will let RetroAchievements players use it instead of locking them into original.

@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-allow-batch branch 2 times, most recently from 472f04f to 615eb86 Compare December 4, 2024 04:31
Data/Sys/GameSettings/G2VE08.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/G2VP08.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/G2VP08.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/G3RP52.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/G4ME69.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GC6J01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GGTE01.ini Outdated Show resolved Hide resolved
# Add action replay cheats here.
$Disable Culling
04011A14 60000001
04011A18 4E800020
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this code works on all versions of the game? If not, it should be in a six-character INI file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The wiki does not specify. https://wiki.dolphin-emu.org/index.php?title=Vexx I am unable to verify this because I don't own a copy of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not comfortable with this unless someone verifies it works in both.

Data/Sys/GameSettings/GK2.ini Show resolved Hide resolved
Data/Sys/GameSettings/GK4.ini Show resolved Hide resolved
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-allow-batch branch 4 times, most recently from 424eb42 to c23e781 Compare December 7, 2024 04:57
Data/Sys/GameSettings/GGTJ01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GGTP01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GHQP7D.ini Show resolved Hide resolved
Data/Sys/GameSettings/GM4E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GM4P01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GPVP01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GSO.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GUNP5D.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GZ2E01.ini Outdated Show resolved Hide resolved
Source/UnitTests/Core/PatchAllowlistTest.cpp Show resolved Hide resolved
New code adds a test failure if there's a Patches/Gecko/AR_Retroachievements_Verified code that doesn't appear to actually exist in the file. This will catch if the allowed patch is formatted wrong, which I found happening several times already due to not realizing that the patch author's name would need to be omitted.
Copy link
Contributor

@JMC47 JMC47 left a comment

Choose a reason for hiding this comment

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

About halfway through looking at things. It mostly looks fine aside from minor nitpicks.

@@ -0,0 +1,40 @@
# G8MJ01 - Paper Mario
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the existing INI has the wrong name, but we should use the official name in all three imo.

@@ -0,0 +1,40 @@
# G8MP01 - Paper Mario
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto here

@@ -0,0 +1,42 @@
# GEZE8P - BillyHatcher
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not for this PR but more and more the old INIs have names that are bugging me.

# Add action replay cheats here.
$Disable Culling
04011A14 60000001
04011A18 4E800020
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not comfortable with this unless someone verifies it works in both.

Adding a community-requested list of Gecko and Action Replay codes to the allowlist. Many of these codes were from the wiki and are being added to Dolphin's repo for the first time.
@LillyJadeKatrin LillyJadeKatrin force-pushed the retroachievements-allow-batch branch from f2305fc to a1f2f3c Compare December 22, 2024 19:01
@JMC47
Copy link
Contributor

JMC47 commented Dec 24, 2024

I scanned through everything and didn't see anything outright blocking, but with this number of files it's entirely possible I might have missed something.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

LGTM except for one file.

@@ -31,3 +31,74 @@ EnableGPUTextureDecoding = False

[Video_Enhancements]
ArbitraryMipmapDetection = True

[Gecko]
$Bloom x4 IR
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be in a three-character INI unless it has been tested to be region free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants