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

Mario Party Cheats Update #9517

Merged
merged 5 commits into from Apr 4, 2021
Merged

Mario Party Cheats Update #9517

merged 5 commits into from Apr 4, 2021

Conversation

EndangeredNayla
Copy link
Contributor

No description provided.

@EndangeredNayla
Copy link
Contributor Author

Update of my older PR #8705 that I polished up and added a few more codes.

@JosJuice
Copy link
Member

It looks like you've changed the line endings to CRLF. Please keep them as LF.

@EndangeredNayla
Copy link
Contributor Author

Thank you for pointing that out

@JMC47
Copy link
Contributor

JMC47 commented Feb 15, 2021

You should be squashing commits so this is only a single commit in the end.

@EndangeredNayla
Copy link
Contributor Author

ill do it but cant GitHub squash commits with a button now

@EndangeredNayla
Copy link
Contributor Author

Any Updates @JosJuice

@EndangeredNayla
Copy link
Contributor Author

This PR adds some new gecko codes to improve the quality of Mario Party on Dolphin. This PR also removes some old codes that are broken and replaced with better similar functioning ones. The codes placed here originally were done by me around a year ago and i would like to revise them. The netplay specific codes are being removed and breaking them down to 3 distinct categories.

@Rumi-Larry
Copy link

I would name the cheats that remove the intro screens to be just that "Remove Minigame Intro Screens" You can change the word remove for "skip". Booting can take a variety of meanings, like booting the game itself or a netplay session.

@EndangeredNayla
Copy link
Contributor Author

EndangeredNayla commented Feb 16, 2021

Remove Minigame Intro Screens

Because it is not minigame screens. This code specifically skips the hudson screen and stuff like that.

@Rumi-Larry
Copy link

Remove Minigame Intro Screens

Because it is not minigame screens. This code specifically skips the hudson screen and stuff like that.

Yeah, I'm dumb...

Data/Sys/GameSettings/GMPE01.ini Show resolved Hide resolved
Data/Sys/GameSettings/GMPE01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GP7E01.ini Outdated Show resolved Hide resolved
@leoetlino
Copy link
Member

leoetlino commented Feb 19, 2021

Could you fix your PR description and commit message to actually describe the changes?

This PR adds some new gecko codes to improve the quality of Mario Party on Dolphin. This PR also removes some old codes that are broken and replaced with better similar functioning ones. The codes placed here originally were done by me around a year ago and i would like to revise them. The netplay specific codes are being removed and breaking them down to 3 distinct categories.

This description you provided sounds good but it should be part of your commit message.

Also, in the future, consider splitting your changes into several independent commits. Having one large monolithic commit with thousands of modified lines and little explanation of what the changes do makes people less willing to review your PR.

@EndangeredNayla
Copy link
Contributor Author

I did originally i was told to squash it

@leoetlino
Copy link
Member

I believe you're misunderstanding. Having a big monolithic commit + fixup commits like "fix line endings" is utterly useless and the fixup commits should definitely be squashed into the relevant commits.

What I meant by splitting your changes is having e.g. one commit that adds the author information, one that adds descriptions to existing codes, another that adds the new codes, etc.

@EndangeredNayla
Copy link
Contributor Author

Oh i see, want me to redo this PR for yall

@leoetlino
Copy link
Member

That'd be great as it'd make the PR more pleasant to review.

@EndangeredNayla
Copy link
Contributor Author

If you are curious the differences between the sections are as such. QOL which is at the top is stuff that is just useful like speeding up animations and stuff.

Board is for tweaking stuff that are part of the board like changing how Pyramid Park operates.

Extra and Game are a little tricky to explain but to easily explain Game is stuff to tweak the board and Extras is ASM hacks to completely change the gameplay

Minigame is to tweak minigames and Minigame Replacement is to replace minigames from ever appearing.

@EndangeredNayla
Copy link
Contributor Author

Is this what you are looking for?

@leoetlino
Copy link
Member

Yep, that's a lot nicer, thanks! Just FYI, there's a typo in one of your commit messages ("Remove broken cheats and merge some togeather" -> together). Also, I'd prefix your commit titles with GameINI: (e.g. GameINI: Mario Party 4 - Add Authors to Cheats) to clearly indicate you're editing the game INIs.

@EndangeredNayla
Copy link
Contributor Author

ahh okay

@EndangeredNayla
Copy link
Contributor Author

@leoetlino Requesting another review. I was the original author of these codes for clarification

@leoetlino leoetlino requested a review from JMC47 March 1, 2021 11:02
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GP7E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GMPE01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GP5E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GP5E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/GP7E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Copy link
Contributor

@PatrickFerry PatrickFerry left a comment

Choose a reason for hiding this comment

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

You can then use git reset soft to undo the master branch merge and the second commit with the same title while preserving your changes and then commit again.

checkout the correct branch -> master in this case.
Then you can run something like this and re-commit
git reset --soft 9cc46470a983f718bbe389ac66a052de5ef35e00

Data/Sys/GameSettings/GP7E01.ini Outdated Show resolved Hide resolved
@JMC47
Copy link
Contributor

JMC47 commented Mar 3, 2021

There's a few typos here and there, however the actual codes seem fine. I tested a few random ones just to see if anything broke.

If the typos are fixed and the commit history is fixed, I'm fine with the actual content of this change.

"*Access all board events regarless of Mega / Mini Mushrooms." as an example of a typo.

@EndangeredNayla
Copy link
Contributor Author

If the typos are fixed and the commit history is fixed, I'm fine with the actual content of this change.

You want me the squash the commits

@EndangeredNayla
Copy link
Contributor Author

You can then use git reset soft

Mind if i just push a new commit and squash them all after

@PatrickFerry
Copy link
Contributor

You can then use git reset soft

Mind if i just push a new commit and squash them all after

I don't think we want them all squashed. Ask on IRC, but I would recommend only combining the last 3 together as it stands.

Copy link
Contributor

@PatrickFerry PatrickFerry left a comment

Choose a reason for hiding this comment

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

If you could make these typo fixes.

Then interactively rebase so that these new changes that correct typos etc instead replace the same lines in the older commit that introduced the lines. I hope that makes sense.

Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Show resolved Hide resolved
Data/Sys/GameSettings/RM8E01.ini Outdated Show resolved Hide resolved
@EndangeredNayla
Copy link
Contributor Author

If you could make these typo fixes.

Then interactively rebase so that these new changes that correct typos etc instead replace the same lines in the older commit that introduced the lines. I hope that makes sense.

These were just in the last commit since I was trying to redo all the descriptions to match. I still think all the commits should be squashed but its up to yall.

@EndangeredNayla
Copy link
Contributor Author

Updates?

@leoetlino
Copy link
Member

That last giant commit basically defeats the whole point of splitting your changes into small, independent, easy-to-review commits :/

The typo fixes should go into the relevant commits, but that's not the most problematic aspect of the last commit. Why are brand new codes being added there?

@EndangeredNayla
Copy link
Contributor Author

Can I please just squash all the commits. I created the original pull request and never had to seperate changes there. We really need these codes merged

@Rumi-Larry
Copy link

It's unnecessary to separate the individual changes for each individual ini. You either need it to integrate all the changes for the individual ini's, or to use different commits for the different changes across the files. The first way seems better suited. I'm unsure if this pr can be salvaged to be reformatted like that, you may need to create a third iteration and definitely don't create a commit just integrating all other commits, that's taken care of by the build process.

@JosJuice
Copy link
Member

I'm unsure if this pr can be salvaged to be reformatted like that, you may need to create a third iteration

It is always possible to salvage a PR without having to create a new one. (Well, except for the case where you selected the wrong branch as the merge target, I think... But that's not relevant here.)

@EndangeredNayla
Copy link
Contributor Author

Okay should hopefully be mergable

@EndangeredNayla
Copy link
Contributor Author

Any updates @JosJuice @Rumi-Larry @leoetlino

@Rumi-Larry
Copy link

@NoraTheGamer I don't have merging privileges, but I would merge it as is.

@JMC47
Copy link
Contributor

JMC47 commented Apr 4, 2021

@dolphin-emu-bot rebuild

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.

This greatly organizes and fixes up cheats along with crediting the authors of cheats. It overall improves the grammar though there's a few very minor things. I didn't test every single cheat code but most of these codes already existed.

@leoetlino leoetlino merged commit ccc99eb into dolphin-emu:master Apr 4, 2021
8 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