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

Add Gateway cheat support #2063

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
@makotech222
Copy link
Contributor

makotech222 commented Sep 10, 2016

This commit adds cheat support for Citra, as well as a GUI to support it.

Cheats can be found at http://www.fort42.com/gateshark/

Cheats are loaded up upon game start. It will read the respective cheat file (named by game id) and parse it into distinct cheats. From there, cheats will be executed 60 times per second.

Things i wanted to discuss before merging:

  • Cheat files are currently located in /citraMainDir/User/Cheats. Can be changed if needed.
  • Tick rate for cheat execution. I dunno if it should be 60/sec.
  • Cheat file format. I tried to make it simple to drag and drop cheats from Fort42, where cheat format is:

[CheatTitle]
*Notes
*Notes2
00111111 11111111

  • General GUI discussion. Let me know if you want any improvements.

Here is a screenshot of the GUI:
image

Still missing from this PR:

  • Support for Joker codes. Won't be completed until InputCore PR is finished

Attached is an example cheat file for Rune Factory 4. So far all cheats i tested in RF4 are working.
00040000000D2800.txt

Special thanks to Desmume emulator, which implemented most of the cheat instructions already.

Edit: Here is a link to the cheat formats and descriptions: http://www.maxconsole.com/threads/gateway-arcode-cheat-sheet.40381/


This change is Reviewable

Edit2: Games tested and working with cheats for hardware 3ds:

  • Rune Factory 4
  • Legend of Zelda OOT
  • Pokemon Omega Ruby
  • Fantasy Life
  • Story of Seasons

Not working:

  • Bravely Default
@neobrain

This comment has been minimized.

Copy link
Member

neobrain commented Sep 10, 2016

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2016-09-10T19:55:32Z: 722af07...makotech222:9d9269d89383c6d3942cf3de0678d09afc5bfe3a

@neobrain

This comment has been minimized.

Copy link
Member

neobrain commented Sep 10, 2016

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2016-09-10T20:23:37Z: 722af07...makotech222:bda26cba1f015ef191cafdb743854185c2ac0c96

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Sep 11, 2016

@poutros Not in this PR. I made it easy to add in though, if someone else wants to do it.

@wwylele

This comment has been minimized.

Copy link
Member

wwylele commented Sep 11, 2016

Could you please provide a document on the cheat system, especially on

  • The format of the cheat code
  • How it works on real hardware (determines the best way to emulate it)

so that people don't use cheat code at all can still review it.


Code style issue: we don't indent in a namespace.


std::string RTrim(std::string &s);

std::string Trim(std::string &s);

This comment has been minimized.

Copy link
@wwylele

wwylele Sep 11, 2016

Member

All these should take const std::string& s as parameter. This fails gcc/clang build

#include <iomanip>
#include <locale>

This comment has been minimized.

Copy link
@wwylele

wwylele Sep 11, 2016

Member

These are not necessary to be exposed in the header file. Put them into the .cpp file

void OnDelete();
};

class QDialogEx : public QDialog {

This comment has been minimized.

Copy link
@wwylele

wwylele Sep 11, 2016

Member

The name is not descriptive. Need an explain on what it is.

@@ -24,7 +24,7 @@ namespace Loader {

static const int kMaxSections = 8; ///< Maximum number of sections (files) in an ExeFs
static const int kBlockSize = 0x200; ///< Size of ExeFS blocks (in bytes)

u64_le program_id = 0;

This comment has been minimized.

Copy link
@wwylele

wwylele Sep 11, 2016

Member

Can be just u64. Also I feel that make this global is not a good idea, but haven't come up with a better one.

@neobrain

This comment has been minimized.

Copy link
Member

neobrain commented Sep 11, 2016

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2016-09-11T03:27:42Z: 722af07...makotech222:6fc4349b9127705e23657ef015a41e0c7faf844a

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Sep 11, 2016

@wwylele Ah i knew I forgot something. I edited OP to link to the page with cheat format and description. It works by basically just doing read/writes to memory addresses.

@neobrain

This comment has been minimized.

Copy link
Member

neobrain commented Sep 11, 2016

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2016-09-11T04:27:16Z: 722af07...makotech222:21d2a90b8951537f7d6e7ad2530060ed8067857e

@mailwl

This comment has been minimized.

Copy link
Contributor

mailwl commented Sep 11, 2016

i like GW search cheat function. @makotech222 will you plan to add it (other PR of course)

@KillzXGaming

This comment has been minimized.

Copy link

KillzXGaming commented Sep 11, 2016

I would love to see a option to download a list automatically similar to how dolphin does with gecko codes. This might be possible with using the title ID as that can get the exact region of the game and which codes to use specifically for that title/region.

If not possible then Citra also could have premade cheat files added in to the downloads of the master just like how dolphin had settings premade.

@emmauss

This comment has been minimized.

Copy link
Contributor

emmauss commented Sep 11, 2016

that is gonna be difficult. currently the only database for arcodes for the 3ds is fort42.com. you can download txt cheat files from there, but:

  1. you need to login first.
  2. all download links are behind adflies and are generated based on last requested game,

dolphin got its codes from http://geckocodes.org ,which has a simple way of requsting code downloads
http://geckocodes.org/txt.php?txt=

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Sep 11, 2016

Review status: 0 of 15 files reviewed at latest revision, 22 unresolved discussions.


src/citra_qt/cheat_gui.h, line 17 at r4 (raw file):

class QWidget;
namespace Ui {
    class CheatDialog;

We don't indent on namespaces. This was explained to you at minimum six times in the previous PR.


src/citra_qt/cheat_gui.h, line 25 at r4 (raw file):

explicit CheatDialog(QWidget* parent = 0);

explicit CheatDialog(QWidget* parent = nullptr);

src/citra_qt/cheat_gui.h, line 36 at r4 (raw file):

    void LoadCheats();

    private slots:

This doesn't need an indent.


src/common/string_util.cpp, line 507 at r4 (raw file):

}

std::string LTrim(std::string & s) {

ampersand against the type. This was also explained quite thoroughly over and over in previous PRs.


src/common/string_util.cpp, line 509 at r4 (raw file):

std::string LTrim(std::string & s) {
    s.erase(s.begin(), std::find_if(s.begin(), s.end(),
        std::not1(std::ptr_fun<int, int>(std::isspace))));

std::not1 and std::ptr_fun are deprecated standard library functions.


src/common/string_util.cpp, line 513 at r4 (raw file):

}

std::string RTrim(std::string & s) {

ditto


src/common/string_util.cpp, line 519 at r4 (raw file):

}

std::string Trim(std::string & s) {

ditto


src/core/cheat_core.cpp, line 36 at r4 (raw file):

    cheat_engine = std::make_unique<CheatEngine::CheatEngine>();
}
}
} // namespace CheatCore

src/core/cheat_core.cpp, line 39 at r4 (raw file):

namespace CheatEngine {
    CheatEngine::CheatEngine() {

We don't indent on namespaces.


src/core/cheat_core.h, line 17 at r4 (raw file):

    void Init();
    void Shutdown();
    void RefreshCheats();

We don't indent on namespaces.


src/core/cheat_core.h, line 21 at r4 (raw file):

namespace CheatEngine {
    /*
    * Represents a single line of a cheat, i.e. 1xxxxxxxx yyyyyyyy

The comment it misaligned


src/core/cheat_core.h, line 23 at r4 (raw file):

    * Represents a single line of a cheat, i.e. 1xxxxxxxx yyyyyyyy
    */
    struct CheatLine {

We don't indent on namespaces


src/core/cheat_core.h, line 24 at r4 (raw file):

    */
    struct CheatLine {
        CheatLine(std::string line) {

explicit CheatLine(std::string line)


src/core/cheat_core.h, line 32 at r4 (raw file):

                return;
            }
            try {

Actually handle the string don't use exceptions for this.


src/core/cheat_core.h, line 56 at r4 (raw file):

     * Base Interface for all types of cheats.
     */
    class ICheat {

Don't use I to signify interfaces. This is a convention in C# and Java, not C++.


src/core/cheat_core.h, line 82 at r4 (raw file):

        void Execute() override;
        std::string ToString() override;
    private:

If there are no private members, remove this.


src/core/cheat_core.h, line 85 at r4 (raw file):

    };

    /*

Misaligned comment


src/core/loader/ncch.h, line 162 at r4 (raw file):

namespace Loader {
    extern u64_le program_id;

Absolutely not. Please provide an actual API to change this instead of using a global. Preferably with rationale in the change.


Comments from Reviewable

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Sep 11, 2016

pointed out some basic things.


Review status: 0 of 15 files reviewed at latest revision, 22 unresolved discussions.


Comments from Reviewable

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Sep 11, 2016

@emmauss Well, I'm still thinking about adding a '.db' file, which would basically be a snapshot of all the codes currently on that site, which we could package with citra, or just host it somewhere. Otherwise, we could go the extra mile and do a html parser for the cheats upon clicking a button or something.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Sep 11, 2016

@lioncash Thank you for the review. Apologies for the common errors, I thought i got them this time :(

  1. regarding program_id: I'm not sure how to handle this one. Would a simple function to getProgramID be okay?
  2. The Trim functions were ripped from stackoverflow. Any suggestion on what to replace the deprecated functions with?
@Subv

This comment has been minimized.

Copy link
Member

Subv commented Sep 12, 2016

that is gonna be difficult. currently the only database for arcodes for the 3ds is fort42.com

We could host our own cheat database, maintained by users and/or dedicated maintainers.

regarding program_id: I'm not sure how to handle this one. Would a simple function to getProgramID be okay?

There is Process::program_id

@makotech222 makotech222 force-pushed the makotech222:CheatsModule branch from 21d2a90 to 72ac603 Oct 13, 2016

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Oct 13, 2016

Okay finally had some time tonight to fix up the issues. I think i should have gotten most if not all.

Edit: Oh except for the try catch around the string handling. SE says its the best way to handle any problems when converting string to base16 numbers. Should I do it some other way?

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Oct 13, 2016

Review status: 0 of 15 files reviewed at latest revision, 82 unresolved discussions, some commit checks failed.


src/citra_qt/cheat_gui.cpp, line 4 at r5 (raw file):

#include <QLineEdit>
#include <QTableWidgetItem>
#include <qcheckbox.h>

#include <QCheckBox> this should also come before QComboBox.


src/citra_qt/cheat_gui.cpp, line 6 at r5 (raw file):

#include <qcheckbox.h>

#include "cheat_gui.h"

This include should have a relative path like the others.


src/citra_qt/cheat_gui.cpp, line 8 at r5 (raw file):

#include "cheat_gui.h"
#include "core/loader/ncch.h"
#include "core/hle/kernel/process.h"

h comes before l


src/citra_qt/cheat_gui.cpp, line 11 at r5 (raw file):

#include "ui_cheat_gui.h"

CheatDialog::CheatDialog(QWidget* parent) : QDialog(parent), ui(new Ui::CheatDialog) {

std::make_unique


src/citra_qt/cheat_gui.cpp, line 12 at r5 (raw file):

CheatDialog::CheatDialog(QWidget* parent) : QDialog(parent), ui(new Ui::CheatDialog) {

Unnecessary newline.


src/citra_qt/cheat_gui.cpp, line 27 at r5 (raw file):

    char buffer[50];
    auto a = sprintf(buffer, "%016llX", Kernel::g_current_process->codeset->program_id);
    auto game_id = std::string(buffer);

You can just use Common::StringFromFormat instead of sprintf and a buffer.

i.e.

const auto game_id = Common::StringFromFormat("%016llX", Kernel::g_current_process->codeset->program_id));

src/citra_qt/cheat_gui.cpp, line 28 at r5 (raw file):

    auto a = sprintf(buffer, "%016llX", Kernel::g_current_process->codeset->program_id);
    auto game_id = std::string(buffer);
    ui->labelTitle->setText("Title ID: " + QString::fromStdString(game_id));

This should be a localizable string.

ui->labelTitle->setText(tr("Title ID: %1").arg(QString::fromStdString(game_id)));

src/citra_qt/cheat_gui.cpp, line 30 at r5 (raw file):

    ui->labelTitle->setText("Title ID: " + QString::fromStdString(game_id));

    connect(ui->buttonClose, SIGNAL(released()), this, SLOT(OnCancel()));

You can just do


src/citra_qt/cheat_gui.cpp, line 50 at r5 (raw file):

    ui->tableCheats->setRowCount(cheats.size());

    for (int i = 0; i < cheats.size(); i++) {

i should be a size_t.


src/citra_qt/cheat_gui.cpp, line 51 at r5 (raw file):

    for (int i = 0; i < cheats.size(); i++) {
        auto enabled = new QCheckBox("");

"" isn't necessary in the constructor.


src/citra_qt/cheat_gui.cpp, line 52 at r5 (raw file):

    for (int i = 0; i < cheats.size(); i++) {
        auto enabled = new QCheckBox("");
        enabled->setCheckState(cheats[i]->enabled ? Qt::CheckState::Checked
enabled->setChecked(cheats[i]->enabled);

src/citra_qt/cheat_gui.cpp, line 55 at r5 (raw file):

                                                  : Qt::CheckState::Unchecked);
        enabled->setStyleSheet("margin-left:7px;");
        ui->tableCheats->setItem(i, 0, new QTableWidgetItem(""));

"" isn't necessary in the constructor.


src/citra_qt/cheat_gui.cpp, line 69 at r5 (raw file):

    CheatEngine::CheatEngine::Save(cheats);
    CheatCore::RefreshCheats();
    this->close();

this-> isn't necessary.


src/citra_qt/cheat_gui.cpp, line 73 at r5 (raw file):

void CheatDialog::OnCancel() {
    this->close();

Ditto.


src/citra_qt/cheat_gui.cpp, line 90 at r5 (raw file):

    ui->textDetails->setEnabled(true);
    ui->textNotes->setEnabled(true);
    auto current_cheat = cheats[row];
const auto& current_cheat = cheats[row];

src/citra_qt/cheat_gui.cpp, line 94 at r5 (raw file):

    std::vector<std::string> details;
    for (auto& line : current_cheat->cheat_lines)

const auto&


src/citra_qt/cheat_gui.cpp, line 117 at r5 (raw file):

    Common::SplitString(details.toStdString(), '\n', detail_lines);
    cheats[current_row]->cheat_lines.clear();
    for (auto& line : detail_lines) {

const auto&


src/citra_qt/cheat_gui.cpp, line 118 at r5 (raw file):

    cheats[current_row]->cheat_lines.clear();
    for (auto& line : detail_lines) {
        cheats[current_row]->cheat_lines.push_back(CheatEngine::CheatLine(line));
cheats[current_row]->cheat_lines.emplace_back(line);

src/citra_qt/cheat_gui.cpp, line 123 at r5 (raw file):

void CheatDialog::OnCheckChanged(int state) {
    QCheckBox* checkbox = qobject_cast<QCheckBox*>(sender());

const QCheckBox*


src/citra_qt/cheat_gui.cpp, line 138 at r5 (raw file):

        rows.push_back(row);
    }
    for (int i = 0; i < rows.size(); i++)
for (int row : rows)
    ui->tableCheats->removeRow(row);

src/citra_qt/cheat_gui.cpp, line 146 at r5 (raw file):

void CheatDialog::OnAddCheat() {
    QDialogEx* dialog = new QDialogEx(this);

This can just be a stack-based dialog. i.e.

QDialogEx dialog;

src/citra_qt/cheat_gui.cpp, line 154 at r5 (raw file):

        return;
    cheats.push_back(result);
    int i = cheats.size() - 1;

please use a more descriptive name than i.

This should also be ... = static_cast<int>(cheats.size() - 1);


src/citra_qt/cheat_gui.cpp, line 155 at r5 (raw file):

    cheats.push_back(result);
    int i = cheats.size() - 1;
    auto enabled = new QCheckBox("");

"" isn't necessary in the constructor.


src/citra_qt/cheat_gui.cpp, line 159 at r5 (raw file):

    enabled->setCheckState(Qt::CheckState::Unchecked);
    enabled->setStyleSheet("margin-left:7px;");
    ui->tableCheats->setItem(i, 0, new QTableWidgetItem(""));

"" isn't necessary in the constructor.


src/citra_qt/cheat_gui.cpp, line 169 at r5 (raw file):

    delete dialog;
}
QDialogEx::QDialogEx(QWidget* parent) : QDialog(parent), ui(this) {

Why is uinecessary as a variable?


src/citra_qt/cheat_gui.cpp, line 170 at r5 (raw file):

}
QDialogEx::QDialogEx(QWidget* parent) : QDialog(parent), ui(this) {
    this->resize(250, 150);

this-> isn't necessary, ditto below).


src/citra_qt/cheat_gui.cpp, line 174 at r5 (raw file):

    auto mainLayout = new QVBoxLayout(this);

    QHBoxLayout* panel = new QHBoxLayout();

Please use something more descriptive than panel, panel2 and panel3


src/citra_qt/cheat_gui.cpp, line 177 at r5 (raw file):

    nameblock = new QLineEdit();
    QLabel* nameLabel = new QLabel();
    nameLabel->setText("Name: ");

This should be a localizable string.


src/citra_qt/cheat_gui.cpp, line 183 at r5 (raw file):

    QHBoxLayout* panel2 = new QHBoxLayout();
    auto typeLabel = new QLabel();
    typeLabel->setText("Type: ");

This should be a localizable string


src/citra_qt/cheat_gui.cpp, line 191 at r5 (raw file):

    QHBoxLayout* panel3 = new QHBoxLayout();
    auto buttonOk = new QPushButton();
    buttonOk->setText("Ok");

You can utilize QDialogButtonBox instead of manually adding OK and cancel buttons.


src/citra_qt/cheat_gui.cpp, line 198 at r5 (raw file):

        if (typeSelect->currentIndex() == 0 && Common::Trim(name).length() > 0) {
            return_value = std::make_shared<CheatEngine::GatewayCheat>(
                std::vector<CheatEngine::CheatLine>(), std::vector<std::string>(), false,

You may want to rearrange how GatewayCheat's constructor is set up, or add a new constructor to get rid of boilerplate like this.


src/citra_qt/cheat_gui.h, line 50 at r5 (raw file):

    Q_OBJECT
public:
    explicit QDialogEx(QWidget* parent = 0);

`= nullptr'


src/citra_qt/cheat_gui.h, line 52 at r5 (raw file):

    explicit QDialogEx(QWidget* parent = 0);
    ~QDialogEx();
    std::shared_ptr<CheatEngine::CheatInterface> return_value;

Provide a getter for the return value. This shouldn't be modifiable by anything outside of this class.


src/citra_qt/cheat_gui.h, line 56 at r5 (raw file):

private:
    QDialogEx* ui;
    QLineEdit* nameblock;

name_block


src/citra_qt/cheat_gui.h, line 57 at r5 (raw file):

    QDialogEx* ui;
    QLineEdit* nameblock;
    QComboBox* typeSelect;

type_select


src/citra_qt/cheat_gui.ui, line 145 at r5 (raw file):

      </layout>
    </widget>
    <widget class="QLabel" name="label">

This should have a better name than label, ditto for other similar occurrences


src/common/string_util.cpp, line 507 at r4 (raw file):

Previously, lioncash (Mat M.) wrote…

ampersand against the type. This was also explained quite thoroughly over and over in previous PRs.

If you're going to modify the string but return it, you can just take the string by value, by the way.

src/common/string_util.cpp, line 467 at r5 (raw file):

std::string Trim(std::string& str) {
    // right trim
    while (str.length() > 0 && (str[str.length() - 1] == ' ' || str[str.length() - 1] == '\t'))

You can just build trim off of functions that do the left and right trim respectively, like so:

std::string TrimLeft(const std::string& str, const std::string& delimiters = " \f\n\r\t\v")
{
    return str.substr(str.find_first_not_of(delimiters));
}

std::string TrimRight(const std::string& str, const std::string delimiters = " \f\n\r\t\v")
{
    return str.substr(str.find_last_not_of(delimiters));
}

std::string Trim(const std::string& str, const std::string delimiters = " \f\n\r\t\v")
{
    return TrimLeft(TrimRight(str, delimiters), delimiters);
}

src/common/string_util.cpp, line 483 at r5 (raw file):

        return elements[0];
    default:
        std::ostringstream os;

The default case can be:

std::ostringstream os;
std::copy(elements.begin(), elements.end(),
          std::ostream_iterator<std::string>(os, delim));

// Drop the trailing delimiter.
std::string result = os.str();
result.pop_back();

return result;

src/core/cheat_core.cpp, line 35 at r5 (raw file):

void RefreshCheats() {
    cheat_engine.reset();
    cheat_engine = std::make_unique<CheatEngine::CheatEngine>();

You shouldn't need to completely recreate the actual cheat engine to perform a refresh. This is something the actual class should handle.

The reset is also unnecessary. Just assigning a new instance to a unique_ptr like you do below is basically a reset and an assignment at the same time, for future reference.


src/core/cheat_core.cpp, line 43 at r5 (raw file):

    // Create folder and file for cheats if it doesn't exist
    FileUtil::CreateDir(FileUtil::GetUserPath(D_USER_IDX) + "\\cheats");
    char buffer[50];

This should probably be turned into a function. You have this same code pasted in multiple places. It can also be simplified.

static std::string GetGameCheatDirectory()
{
    const auto program_id = Common::StringFromFormat("%016llX", Kernel::g_current_process->codeset->program_id);

    return FileUtil::GetUserPath(D_USER_IDX) + "\\cheats\\" + program_id + ".txt";
}

src/core/cheat_core.cpp, line 70 at r5 (raw file):

    std::string name;
    bool enabled = false;
    for (int i = 0; i < lines.size(); i++) {

i should be a size_t.


src/core/cheat_core.cpp, line 73 at r5 (raw file):

        std::string current_line = std::string(lines[i].c_str());
        current_line = Common::Trim(current_line);
        if (code_type == "")

if (code_type.empty())


src/core/cheat_core.cpp, line 75 at r5 (raw file):

        if (code_type == "")
            continue;
        if (current_line.substr(0, 2) == "+[") { // Enabled code

You can use std::string's compare function instead of creating a new string.


src/core/cheat_core.cpp, line 76 at r5 (raw file):

            continue;
        if (current_line.substr(0, 2) == "+[") { // Enabled code
            if (cheat_lines.size() > 0) {

if (!cheat_lines.empty())


src/core/cheat_core.cpp, line 86 at r5 (raw file):

            enabled = true;
            continue;
        } else if (current_line.substr(0, 1) == "[") { // Disabled code
else if (current_line.front() == '[')

src/core/cheat_core.cpp, line 87 at r5 (raw file):

            continue;
        } else if (current_line.substr(0, 1) == "[") { // Disabled code
            if (cheat_lines.size() > 0) {

if (!cheat_lines.empty())


src/core/cheat_core.cpp, line 119 at r5 (raw file):

        FileUtil::GetUserPath(D_USER_IDX) + "\\cheats\\" + std::string(buffer) + ".txt";
    FileUtil::IOFile file = FileUtil::IOFile(file_path, "w+");
    bool sectionGateway = false;

This variable is unused.


src/core/cheat_core.cpp, line 122 at r5 (raw file):

    for (auto& cheat : cheats) {
        if (cheat->type == "Gateway") {
            file.WriteBytes(cheat->ToString().c_str(), cheat->ToString().length());

the result of cheat->ToString();should be stored to a variable, otherwise you do unnecessary work a second time.


src/core/cheat_core.cpp, line 125 at r5 (raw file):

        }
    }
    file.Close();

Close() isn't necessary this will be done automatically in IOFile's destructor.


src/core/cheat_core.cpp, line 137 at r5 (raw file):

    if (enabled == false)
        return;
    u32 addr = 0;

Please declare these variables at the scopes they're used in.


src/core/cheat_core.cpp, line 146 at r5 (raw file):

    u32 counter = 0;
    bool loop_flag = false;
    for (int i = 0; i < cheat_lines.size(); i++) {

i should be a size_t. This is what size() returns.


src/core/cheat_core.cpp, line 154 at r5 (raw file):

        if (if_flag > 0) {
            if (line.type == 0x0E)
                i += ((line.value + 7) / 8);

The outer set of parentheses are unnecessary.


src/core/cheat_core.cpp, line 160 at r5 (raw file):

            {
                if (loop_flag)
                    i = (loopbackline - 1);

Parentheses do nothing but add line noise here.


src/core/cheat_core.cpp, line 305 at r5 (raw file):

            case 0x01: {
                if (loop_flag)
                    i = (loopbackline - 1);

ditto


src/core/cheat_core.cpp, line 310 at r5 (raw file):

            case 0x02: {
                if (loop_flag)
                    i = (loopbackline - 1);

ditto.


src/core/cheat_core.cpp, line 385 at r5 (raw file):

std::string GatewayCheat::ToString() {
    std::string result = "";

= "";

Isn't necessary. Strings initialize to empty by default.


src/core/cheat_core.cpp, line 386 at r5 (raw file):

std::string GatewayCheat::ToString() {
    std::string result = "";
    if (cheat_lines.size() == 0)

if (cheat_lines.empty())


src/core/cheat_core.cpp, line 389 at r5 (raw file):

        return result;
    if (enabled)
        result += "+";

This can be a character literal instead of a string literal.


src/core/cheat_core.cpp, line 390 at r5 (raw file):

    if (enabled)
        result += "+";
    result += "[" + name + "]\n";

"[" can be a character literal instead of a string literal.


src/core/cheat_core.cpp, line 392 at r5 (raw file):

    result += "[" + name + "]\n";
    for (auto& str : notes) {
        if (str.substr(0, 1) != "*")

You can use std::string's compare function to do this without constructing a new string all the time.


src/core/cheat_core.cpp, line 393 at r5 (raw file):

    for (auto& str : notes) {
        if (str.substr(0, 1) != "*")
            str.insert(0, "*");

You can use a character literal '*' instead of a string literal here.


src/core/cheat_core.cpp, line 396 at r5 (raw file):

    }
    result += Common::Join(notes, "\n");
    if (notes.size() > 0)

if (!notes.empty())


src/core/cheat_core.cpp, line 397 at r5 (raw file):

    result += Common::Join(notes, "\n");
    if (notes.size() > 0)
        result += "\n";

You can just use the character '\n' instead of a string literal. Ditto where applicable below.


src/core/cheat_core.cpp, line 398 at r5 (raw file):

    if (notes.size() > 0)
        result += "\n";
    for (auto& line : cheat_lines)

const auto& line


src/core/cheat_core.h, line 12 at r5 (raw file):

#include "core/core_timing.h"
/*
 * Starting point for running cheat codes. Initializes tick event and executes cheats every tick.

This should be placed on the relevant function.


src/core/cheat_core.h, line 27 at r5 (raw file):

        line = std::string(line.c_str()); // remove '/0' characters if any.
        line = Common::Trim(line);
        if (line.length() != 17) {

You should make 17 a constexpr constant to explain the significance of the number.


src/core/cheat_core.h, line 33 at r5 (raw file):

        }
        try {
            type = stoi(line.substr(0, 1), 0, 16);

std::stoi same for below.


src/core/cheat_core.h, line 34 at r5 (raw file):

        try {
            type = stoi(line.substr(0, 1), 0, 16);
            if (type == 0xD)

You should either have a self-descriptive constant variable, or a comment here explaining this.


src/core/cheat_core.h, line 55 at r5 (raw file):

 * Base Interface for all types of cheats.
 */
class CheatInterface {

CheatBase is probably a better name here, since CheatInterface sounds like something that handles actual cheats.


src/core/cheat_core.h, line 61 at r5 (raw file):

    virtual std::string ToString() = 0;

    std::vector<std::string> notes;

These should be private or protected with appropriate accessors.


src/core/cheat_core.h, line 63 at r5 (raw file):

    std::vector<std::string> notes;
    bool enabled;
    std::string type;

You can probably just use an enum class for this sort of thing. You wouldn't have to do a string compare all the time to determine the type.


src/core/cheat_core.h, line 72 at r5 (raw file):

class GatewayCheat : public CheatInterface {
public:
    GatewayCheat(std::vector<CheatLine> _cheatlines, std::vector<std::string> _notes, bool _enabled,

Underscores as a prefix are reserved by the standard, if you need an underscore place at the end of the variable.

This constructor can also utilize move assignment as well.

GatewayCheat(std::vector<CheatLine> cheat_lines_, std::vector<std::string> notes_, bool enabled_,
             std::string _name) {
    cheat_lines = std::move(cheat_lines_);
    notes = std::move(notes);
    enabled = enabled_;
    name = std::move(name);
    type = "Gateway";
}

Comments from Reviewable

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Oct 13, 2016

Review status: 0 of 15 files reviewed at latest revision, 83 unresolved discussions, some commit checks failed.


src/citra_qt/cheat_gui.cpp, line 30 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can just do

You can just use the new Qt signal/slot syntax described [here](https://wiki.qt.io/New_Signal_Slot_Syntax) *

src/core/cheat_core.cpp, line 97 at r5 (raw file):

            enabled = false;
            continue;
        } else if (current_line.substr(0, 1) == "*") { // Comment
else if (current_line.front() == '*')

src/core/cheat_core.cpp, line 392 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can use std::string's compare function to do this without constructing a new string all the time.

Actually, this one can be `if (str.front() == '*')`

Comments from Reviewable

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Oct 15, 2016

Okay I think i fixed just about all of the issues. Thanks again @lioncash for taking time to go through my code.

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Oct 15, 2016

Review status: 0 of 15 files reviewed at latest revision, 46 unresolved discussions.


src/citra_qt/cheat_gui.cpp, line 11 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

std::make_unique

This still applies.

src/citra_qt/cheat_gui.cpp, line 169 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

Why is uinecessary as a variable?

You still didn't answer this.

src/citra_qt/cheat_gui.cpp, line 191 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can utilize QDialogButtonBox instead of manually adding OK and cancel buttons.

This still applies.

src/citra_qt/cheat_gui.cpp, line 59 at r6 (raw file):

            i, 2, new QTableWidgetItem(QString::fromStdString(cheats[i]->GetType())));
        enabled->setProperty("row", i);
        connect(enabled, SIGNAL(stateChanged(int)), this, SLOT(OnCheckChanged(int)));

This can use Qt's new event syntax as well.


src/citra_qt/cheat_gui.cpp, line 152 at r6 (raw file):

        return;
    cheats.push_back(result);
    int newCheatIndex = static_cast<int>(cheats.size() - 1);

The variable naming convention is snake_case. This is something that's in the contributor guidelines.


src/citra_qt/cheat_gui.h, line 47 at r2 (raw file):

Previously, wwylele wrote…

The name is not descriptive. Need an explain on what it is.

@wylele's comment still applies.

src/citra_qt/cheat_gui.h, line 29 at r6 (raw file):

private:
    Ui::CheatDialog* ui;

This can be a unique_ptr.


src/citra_qt/cheat_gui.h, line 52 at r6 (raw file):

    explicit QDialogEx(QWidget* parent = nullptr);
    ~QDialogEx();
    std::shared_ptr<CheatEngine::CheatBase> GetReturnValue() {

std::shared_ptr<CheatEngine::CheatBase> GetReturnValue() const {


src/citra_qt/cheat_gui.ui, line 145 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

This should have a better name than label, ditto for other similar occurrences

This still applies to the layouts.

src/common/string_util.cpp, line 467 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can just build trim off of functions that do the left and right trim respectively, like so:

std::string TrimLeft(const std::string& str, const std::string& delimiters = " \f\n\r\t\v")
{
    return str.substr(str.find_first_not_of(delimiters));
}

std::string TrimRight(const std::string& str, const std::string delimiters = " \f\n\r\t\v")
{
    return str.substr(str.find_last_not_of(delimiters));
}

std::string Trim(const std::string& str, const std::string delimiters = " \f\n\r\t\v")
{
    return TrimLeft(TrimRight(str, delimiters), delimiters);
}
This still applies.

src/core/cheat_core.cpp, line 392 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

Actually, this one can be if (str.front() == '*')

this still applies.

src/core/cheat_core.cpp, line 393 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can use a character literal '*' instead of a string literal here.

This still applies.

src/core/cheat_core.cpp, line 19 at r6 (raw file):

static void CheatTickCallback(u64, int cycles_late) {
    if (cheat_engine == nullptr)

Out of curiosity, why is the engine not instantiated in the Init function and freed in the Shutdown function? This makes the application use memory when it isn't necessary when the core is shutdown as is. It also introduces an unnecessary check in the callback.


src/core/cheat_core.cpp, line 96 at r6 (raw file):

            continue;
        } else if (current_line.front() == '*') { // Comment
            notes.push_back(current_line);

notes.push_back(std::move(current_line));


src/core/cheat_core.cpp, line 98 at r6 (raw file):

            notes.push_back(current_line);
        } else if (!current_line.empty()) {
            cheat_lines.push_back(CheatLine(current_line));

cheat_lines.emplace_back(std::move(current_line))


src/core/cheat_core.cpp, line 101 at r6 (raw file):

        }
        if (i == lines.size() - 1) { // End of file
            if (cheat_lines.size() > 0) {

!cheat_lines.empty()


src/core/cheat_core.cpp, line 114 at r6 (raw file):

    char buffer[50];
    auto a = sprintf(buffer, "%016llX", Kernel::g_current_process->codeset->program_id);
    std::string file_path =

You can use GetFilePath here.


src/core/cheat_core.h, line 34 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You should either have a self-descriptive constant variable, or a comment here explaining this.

Comments should go on top of if-statements, not on the side.

src/core/cheat_core.h, line 63 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You can probably just use an enum class for this sort of thing. You wouldn't have to do a string compare all the time to determine the type.

This still applies.

src/core/cheat_core.h, line 62 at r6 (raw file):

    virtual ~CheatBase() = default;
    virtual std::string ToString() = 0;
    std::vector<std::string>& GetNotes() {

const std::vector<std::string>& GetNotes() const {


src/core/cheat_core.h, line 65 at r6 (raw file):

        return notes;
    }
    void SetNotes(std::vector<std::string> val) {

new_notes


src/core/cheat_core.h, line 66 at r6 (raw file):

    }
    void SetNotes(std::vector<std::string> val) {
        notes = val;

notes = std::move(new_notes);


src/core/cheat_core.h, line 68 at r6 (raw file):

        notes = val;
    }
    bool& GetEnabled() {

This should just be:

bool GetEnabled() const {


src/core/cheat_core.h, line 71 at r6 (raw file):

        return enabled;
    }
    void SetEnabled(bool val) {

value


src/core/cheat_core.h, line 74 at r6 (raw file):

        enabled = val;
    }
    std::string& GetType() {

const std::string& GetType() const {


src/core/cheat_core.h, line 77 at r6 (raw file):

        return type;
    }
    void SetType(std::string val) {

`new_type


src/core/cheat_core.h, line 78 at r6 (raw file):

    }
    void SetType(std::string val) {
        type = val;

type = std::move(new_type);


src/core/cheat_core.h, line 80 at r6 (raw file):

        type = val;
    }
    std::vector<CheatLine>& GetCheatLines() {

const std::vector<CheatLine>& GetCheatLines() const {


src/core/cheat_core.h, line 83 at r6 (raw file):

        return cheat_lines;
    }
    void SetCheatLines(std::vector<CheatLine> val) {

new_lines


src/core/cheat_core.h, line 84 at r6 (raw file):

    }
    void SetCheatLines(std::vector<CheatLine> val) {
        cheat_lines = val;

cheat_lines = std::move(new_lines);


src/core/cheat_core.h, line 86 at r6 (raw file):

        cheat_lines = val;
    }
    std::string& GetName() {

const std::string& GetName() const {


src/core/cheat_core.h, line 89 at r6 (raw file):

        return name;
    }
    void SetName(std::string val) {

new_name


src/core/cheat_core.h, line 90 at r6 (raw file):

    }
    void SetName(std::string val) {
        name = val;

name = std::move(new_name);


src/core/cheat_core.h, line 95 at r6 (raw file):

protected:
    std::vector<std::string> notes;
    bool enabled;

bool enabled = false;


src/core/cheat_core.h, line 106 at r6 (raw file):

public:
    GatewayCheat(std::string name_) {
        cheat_lines = std::vector<CheatLine>();

This constructor can just be

GatewayCheat(std::string name_)
{
    name = std::move(name_);
    type = "Gateway";
}

src/core/cheat_core.h, line 137 at r6 (raw file):

private:
    std::vector<std::shared_ptr<CheatBase>> cheats_list;
    static std::string GetFilePath();

This can be left in the cpp file. This is an implementation detail.


Comments from Reviewable

@lioncash

This comment has been minimized.

Copy link
Member

lioncash commented Oct 15, 2016

Also in some places, you format variables as variableName, but our style guide specifies that they should be formatted as variable_name where possible.

I'd also like to make a somewhat frank (but not with bad intentions in mind) request that you please read up on C++ more. From what I can tell, based off this and previous PRs, you seem to keep treating the language as if it were C#; they may share similarities, but they aren't alike at all.

I continually have to point out the same thing over and over and over and over. Believe me, I get if someone is new to a language, I understand that and will personally provide examples to people both on Github and IRC, and take the time to help them learn the language; if I didn't like doing that I wouldn't be doing review on nearly every PR for Citra and Dolphin in some form. But when it's the same person I have to constantly point out the same thing for, it starts becoming exhausting and feels like I'm talking or rather, reviewing, a brick wall or someone that just doesn't care. There have been times where I've repeatedly pointed out something, or a way to do something that is preferable to current code, only to see the code that was pointed out last time occur again in a newer change.

If I or another reviewer point out something should be like [x] or it should use [language feature] rather than the current way a piece of code is structured, but you don't know what that feature is or how it works, look it up and try and understand what it is or does. There's also people on IRC that can help you to understand it.

I'm not saying this just to take it out on you for the sake of doing so, but because it's been a common theme on almost all PR's that I've had to review by you, and like I said, it gets exhausting over time.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Oct 15, 2016

Apologies @lioncash. I appreciate your patience with me. Indeed I exclusively write in c# 99% of the time, except for when i work on citra, so sometimes my C# code style shows up without me realizing it.

Regarding the field accessors, with the gets, don't I want them to be modifiable from outside the object? For example on the cheat_lines, i want to be able to call .clear() and such.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Oct 15, 2016

Review status: 0 of 16 files reviewed at latest revision, 46 unresolved discussions, some commit checks failed.


src/citra_qt/cheat_gui.cpp, line 169 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

You still didn't answer this.

Not entirely sure. I must've missed this one.

src/citra_qt/cheat_gui.cpp, line 191 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

This still applies.

Is it necessary? Seems easier to just add buttons.

src/common/string_util.cpp, line 467 at r5 (raw file):

Previously, lioncash (Mat M.) wrote…

This still applies.

For some reason, you trim functions weren't working properly, so i reverted. Trim left was taking all but last character in a line for some reason. Didn't take any time to debug.

src/core/cheat_core.cpp, line 98 at r6 (raw file):

Previously, lioncash (Mat M.) wrote…

cheat_lines.emplace_back(std::move(current_line))

Curious, whats difference between emplace and push for vector? Also whats std::move for?

Comments from Reviewable

}
}
case 0x0E: { // EXXXXXXX YYYYYYYY Copy YYYYYYYY parameter bytes to [XXXXXXXX+offset...]
// TODO: Implement whatever this is...

This comment has been minimized.

Copy link
@JayFoxRox

JayFoxRox Dec 8, 2016

Member

Shouldn't there be a warning at least?
Same for 0xD

That warning should also happen only once on loading / parsing, not on execution?!

@emmauss

This comment has been minimized.

Copy link
Contributor

emmauss commented Dec 8, 2016

This is something i have noticed. XXXXXXXX EYYYYYYY format cheats dont work well, it is like a random value has be applied at their respective address, causing the cheats to go haywire.

edit: they were actually joker codes, with formats
DXXXXXXX YYYYYYYY
00000000 EYYYYYYY
OR
DXXXXXXX EYYYYYYY
didnt notice it at first. but shoudnt they like, not execute, since you break at that point.

@emmauss

This comment has been minimized.

Copy link
Contributor

emmauss commented Dec 8, 2016

another comment, from the documatation page you referenced, joker codes( or 'keypress' codes) uses the 0xDD format. breaking from all occurances of 0xD code may cause codes that arent 'joker' to fail or run differently. this occurs in many pokemon cheats.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Dec 13, 2016

@emmauss IIRC, joker codes will actually execute, not paying any attention to which button was pressed. If i don't implement Joker in this PR, i will at very least prevent joker cheats from running at all.

@yuriks

This comment has been minimized.

Copy link
Member

yuriks commented Dec 14, 2016

I'll be honest here: Is there any point in adding support for this if codes for a 3DS won't reliably work on Citra due to memory layout differences?

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Dec 14, 2016

I've tested only two games, and so far they work 50% of the time. Seems like pokemon games work too. If the 3ds codes don't work, you can still write codes using cheat engine alongside the 3ds codes to find the offset.

@Subv

This comment has been minimized.

Copy link
Member

Subv commented Dec 14, 2016

I don't think there's much value in adding this now, the memory layout will only continue to change and evolve as citra grows older. IMO this should wait until at least multiprocess is implemented.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Dec 14, 2016

But even if the memory layout changes, the code for cheats stays the same, so theres not much point adding it later over adding it now. Besides, it serves as a great test for accuracy on memory layout. At the very least, leave it in bleeding edge so that people can enjoy it.

@JayFoxRox

This comment has been minimized.

Copy link
Member

JayFoxRox commented Dec 21, 2016

No, the cheats strongly depend on the proper 3DS memory layout. As such it's currently not possible to write proper cheat support for Citra. And as you say: Once the memory layout has been fixed we can probably update and integrate this one. However, chances are we'll have a couple of refactors which make it incompatible (and if we merge this now we'll have the burden of maintaining broken / unused code).
New cheats created in Citra might even be incompatible with real 3DS due to the layout differences.

Randomly reading / writing values is also not a great test for the memory layout. It would just lead to undefined behaviour and randomness. A much more solid approach would be to compare the heap structure or dumping the mapped regions on real hardware and Citra.

Also this PR has been absent from Bleeding-Edge again and again (like it is again right now).
With very minor exceptions (like fix-batching, which is reverting a small fix ~ 5 lines?) Bleeding-Edge is supposed to include features which are close to being merged and at least have a chance of being merged soon. We do not want to raise expectations we can't meet with future (nightly) builds, as such, it would be a bad idea to close this but still keep it in Bleeding-Edge.


I did not test this PR yet so I don't want to close this yet. However, @makotech222, I strongly suggest to close this and then focus on fixing your inputcore so it can be merged (in small chunks). It looks like a lot more work went into inputcore and it's a far more important feature.
Two of these large PRs are probably too much to handle for a single person, so you should focus on one of them.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Dec 21, 2016

The cheat module is written for the proper 3ds memory layout. As such, it will likely not change at all over the course of citra development (aside from maybe a global offset). As Citra gets more accurate, more and more games will start to work with the cheat module.

Not sure why bleeding edge doesn't merge correctly with this PR though. I don't really know how that works so i can't comment on it.

If we close the PR, these changes will just sit on the sideline until citra fixes the memory layout. I would prefer they stay, at least in bleeding edge, so that the games that are compatible will be more enjoyable for users (like me)

@makotech222 makotech222 force-pushed the makotech222:CheatsModule branch 3 times, most recently from e594d44 to 4225c7c Jan 7, 2017

@makotech222 makotech222 force-pushed the makotech222:CheatsModule branch from 4225c7c to c09f519 Jan 14, 2017

@xbill321

This comment has been minimized.

Copy link

xbill321 commented Jan 20, 2017

It seems the conditional 16bit codes aren't implemented correctly. It's supposed to apply a bitmask to the value before checking. It's actually written in the comment, but not in the code.

for example

case 0x09: { // 9XXXXXXX ZZZZYYYY   IF YYYY = ((not ZZZZ) AND half[XXXXXXX])
...
val = Memory::Read16(line.address);
if (static_cast<u16>(line.value) == val) 
...

should look like

case 0x09: { // 9XXXXXXX ZZZZYYYY   IF YYYY = ((not ZZZZ) AND half[XXXXXXX])
...
val = Memory::Read16(line.address);
if (static_cast<u16>(line.value) == (!static_cast<u16>(line.value >> 16) & val))
...

It's pretty useful for games that store multiple unrelated values in the same byte.

@makotech222 makotech222 force-pushed the makotech222:CheatsModule branch from c09f519 to 6b61b51 Feb 1, 2017

Anon Anon

@makotech222 makotech222 force-pushed the makotech222:CheatsModule branch from 067e225 to 3c01d6a Feb 1, 2017

Anon added some commits Feb 1, 2017

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Feb 1, 2017

Okay pushed changes to support 0xE codes. Also fixed 0x09 codes according to @xbill321 , thanks!

Also updated OP with games i've tested as working and not working. Let me know if i should add any other games that you guys have tested.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Feb 2, 2017

Okay i think all issues are just about fixed now, except for Joker commands. Still waiting for Input overhaul to merge before doing anything on that.

@yuriks

This comment has been minimized.

Copy link
Member

yuriks commented Feb 13, 2017

While we appreciate the amount of effort that went into this feature, we've decided to not accept it in Citra at this time, for several reasons:

  • Due to imperfections in Citra's emulation, the memory addresses don't match up to what you'd find in a real 3DS. This isn't the fault of this PR, but it means that for some games you need a separate set of codes for use with Citra. We don't want these codes to spread and cause confusion later, especially since the layout may change unpredictably between versions.
  • Using cheats occasionally causes games to break, and this has caused additional support burden for our moderators in the forum when people come ask for help with bugs that are caused by broken cheat codes.
  • It's not a core emulation or debugging feature, which is what we want to focus on right now until we achieve better compatibility and accuracy.

These are things that will certainly change in the future, and when that time comes, this will be considered again. You're of course free to continue to work on the branch yourself or to attempt to contribute it again when the project is more mature. I'm sorry for doing this after so much work has gone into the branch, I should've made this clearer earlier on.

@yuriks yuriks closed this Feb 13, 2017

@MerryMage

This comment has been minimized.

Copy link
Member

MerryMage commented Jul 17, 2018

Note for people from the future who intend to pick this up: You will need to InvalidateCacheRange to ensure code modifications go through.

@makotech222

This comment has been minimized.

Copy link
Contributor Author

makotech222 commented Jul 18, 2018

@MerryMage Thanks for the headsup. Is this a recent change that has been merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.