Skip to content

Memory Observer improvements: #1927

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.

Sign up for GitHub

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

Open
notdodgeball wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Memory Observer improvements: #1927

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
943c847
Memory Observer improvements:
notdodgeball Apr 24, 2025
51abf1a
Merge branch 'grumpycoders:main' into memory-observer
notdodgeball Jul 31, 2025
7609e9f
Merge branch 'grumpycoders:main' into memory-observer
notdodgeball May 28, 2026
0653755
More improvements
notdodgeball May 28, 2026
3ee744c
Merge branch 'main' into memory-observer
nicolasnoble Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 148 additions & 92 deletions src/gui/widgets/memory_observer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,313 +95,369 @@
ImGuiTableFlags_BordersOuter | ImGuiTableFlags_BordersV;

if (ImGui::BeginTabItem(_("Plain search"))) {
bool gotEnter = ImGui::InputText(_("Pattern"), &m_plainSearchString, ImGuiInputTextFlags_EnterReturnsTrue);
bool gotEnter = ImGui::InputText(
_("Pattern"), &m_plainSearchString,
ImGuiInputTextFlags_EnterReturnsTrue |
(m_plainHex ? ImGuiInputTextFlags_CharsHexadecimal : ImGuiInputTextFlags_CharsDecimal));
ImGui::Checkbox(_("Hex"), &m_plainHex);
auto needleSize = 0;
std::string needle;
bool valid = true;
if (m_plainHex) {
char n = 0;
bool gotOne = false;
auto maybePushOne = [&]() {
if (gotOne) {
needle += n;
gotOne = false;
needleSize++;
n = 0;
} else {
gotOne = true;
}
};
for (auto c : m_plainSearchString) {
if (c >= '0' && c <= '9') {
n <<= 4;
n |= c - '0';
maybePushOne();
} else if (c >= 'a' && c <= 'f') {
n <<= 4;
n |= c - 'a' + 10;
maybePushOne();
} else if (c >= 'A' && c <= 'F') {
n <<= 4;
n |= c - 'A' + 10;
maybePushOne();
} else if (c == ' ') {
if (gotOne) {
needle += n;
gotOne = false;
needleSize++;
n = 0;
}
} else {
valid = false;
break;
}
}
if (gotOne) {
needle += n;
needleSize++;
}
} else {
needleSize = m_plainSearchString.size();
needle = m_plainSearchString;
}
if (!valid) {
ImGui::BeginDisabled();
}
if (ImGui::Button(_("Search")) || (gotEnter && valid)) {
auto ptr = memData;
m_plainAddresses.clear();
while (true) {
auto found = reinterpret_cast<const uint8_t*>(
memmem(ptr, memData + memSize - ptr, needle.c_str(), needleSize));
if (found) {
m_plainAddresses.push_back(memBase + static_cast<uint32_t>(found - memData));
ptr = reinterpret_cast<const uint8_t*>(found) + 1;
} else {
break;
}
}
}
if (!valid) {
ImGui::EndDisabled();
}
if (ImGui::BeginTable(_("Found values"), 2, tableFlags)) {
if (ImGui::BeginTable(_("Found values"), 3, tableFlags)) {
ImGui::TableSetupColumn(_("Address"));
ImGui::TableSetupColumn(_("Access"));
ImGui::TableSetupColumn(_("Remove"));
ImGui::TableHeadersRow();

ImGuiListClipper clipper;
clipper.Begin(m_plainAddresses.size());
while (clipper.Step()) {
for (int row = clipper.DisplayStart; row < clipper.DisplayEnd; ++row) {
const auto& currentAddress = m_plainAddresses[row];

ImGui::TableNextRow();
ImGui::TableSetColumnIndex(0);
ImGui::Text("%x", currentAddress);
ImGui::TableSetColumnIndex(1);
auto buttonName = fmt::format(f_("Show in memory editor##{}"), row);
if (ImGui::Button(buttonName.c_str())) {
const uint32_t editorAddress = currentAddress - memBase;
g_system->m_eventBus->signal(
PCSX::Events::GUI::JumpToMemory{editorAddress, static_cast<unsigned>(needleSize)});
}
ImGui::TableSetColumnIndex(2);
auto removeButtonName = fmt::format(f_("╳##{}"), row);
if (ImGui::Button(removeButtonName.c_str())) {
m_plainAddresses.erase(m_plainAddresses.begin() + row);
clipper.Begin(m_plainAddresses.size());
break;
}
}
}
ImGui::EndTable();
}
ImGui::EndTabItem();
}

if (ImGui::BeginTabItem(_("Delta-over-time search"))) {
const auto stride = getStrideFromValueType(m_scanValueType);

if (m_addressValuePairs.empty() && ImGui::Button(_("First scan"))) {
int64_t memValue = 0;

for (uint32_t i = 0; i < memSize; ++i) {
if (i != 0 && i % stride == 0) {
switch (m_scanType) {
case ScanType::ExactValue:
if (memValue == m_value) {
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::GreaterThan:
if (memValue > m_value) {
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::LessThan:
if (memValue < m_value) {
if (ImGui::Button(_("New scan"))) {
if (m_addressValuePairs.empty()) {
int64_t memValue = 0;

for (uint32_t i = 0; i < memSize; ++i) {
if (i != 0 && i % stride == 0) {
switch (m_scanType) {
case ScanType::ExactValue:
if (memValue == m_value) {
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::GreaterThan:
if (memValue > m_value) {
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::LessThan:
if (memValue < m_value) {
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::Changed:
case ScanType::Unchanged:
case ScanType::Increased:
case ScanType::Decreased:
break;
case ScanType::UnknownInitialValue:
m_addressValuePairs.push_back({memBase + i - stride, memValue});
}
break;
case ScanType::Changed:
case ScanType::Unchanged:
case ScanType::Increased:
case ScanType::Decreased:
break;
case ScanType::UnknownInitialValue:
m_addressValuePairs.push_back({memBase + i - stride, memValue});
break;
break;
}

memValue = 0;
}

memValue = 0;
const uint8_t currentByte = memData[i];
const uint8_t leftShift = 8 * (i % stride);
const uint32_t mask = 0xffffffff ^ (0xff << leftShift);
const int byteToWrite = currentByte << leftShift;
memValue = (memValue & mask) | byteToWrite;
memValue = getValueAsSelectedType(memValue);
}
Comment on lines +211 to +252

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Last stride-aligned memory block is never scanned.

The loop processes accumulated memValue at stride boundaries (when i != 0 && i % stride == 0), but the final block accumulated during the last stride iterations is never evaluated after the loop exits. This causes the addresses from memBase + memSize - stride onward to be missing from scan results.

Proposed fix: process the final block after the loop 
                         memValue = getValueAsSelectedType(memValue);
                     }
+                    // Process the final accumulated value
+                    switch (m_scanType) {
+                        case ScanType::ExactValue:
+                            if (memValue == m_value) {
+                                m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+                            }
+                            break;
+                        case ScanType::GreaterThan:
+                            if (memValue > m_value) {
+                                m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+                            }
+                            break;
+                        case ScanType::LessThan:
+                            if (memValue < m_value) {
+                                m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+                            }
+                            break;
+                        case ScanType::Changed:
+                        case ScanType::Unchanged:
+                        case ScanType::Increased:
+                        case ScanType::Decreased:
+                            break;
+                        case ScanType::UnknownInitialValue:
+                            m_addressValuePairs.push_back({memBase + memSize - stride, memValue});
+                            break;
+                    }
                 } else {
                     m_addressValuePairs.clear();

Alternatively, refactor the loop to process after accumulation or extract the switch logic into a helper function to avoid duplication.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gui/widgets/memory_observer.cc` around lines 211 - 252, The final
stride-aligned block never gets evaluated because the switch handling
accumulated memValue only runs when i % stride == 0 inside the loop; after the
loop ends you must run the same switch on the last accumulated memValue (the
block that covers memBase + memSize - stride .. end) and push matches into
m_addressValuePairs; implement this by either extracting the switch logic into a
helper (e.g., ProcessScannedValue(int64_t value, uint32_t addr) and call it
inside the loop and once after the loop) or by duplicating the switch after the
loop using the same conditions (use m_scanType, m_value, memValue, memBase,
stride, getValueAsSelectedType, memData to compute the correct address).

} else {
m_addressValuePairs.clear();
m_scanType = ScanType::ExactValue;
}
}

ImGui::Checkbox(_("Hex"), &m_hex);

if (!m_hex && stride > 1) {
ImGui::SameLine();
ImGui::Checkbox(_("Display as fixed-point values"), &m_fixedPoint);
}

const auto currentScanValueType = magic_enum::enum_name(m_scanValueType);
if (ImGui::BeginCombo(_("Value type"), currentScanValueType.data())) {
for (auto v : magic_enum::enum_values<ScanValueType>()) {
bool selected = (v == m_scanValueType);
auto name = magic_enum::enum_name(v);
if (ImGui::Selectable(name.data(), selected)) {
m_scanValueType = v;
}
if (selected) {
ImGui::SetItemDefaultFocus();
}
}
ImGui::EndCombo();
}

const uint8_t currentByte = memData[i];
const uint8_t leftShift = 8 * (i % stride);
const uint32_t mask = 0xffffffff ^ (0xff << leftShift);
const int byteToWrite = currentByte << leftShift;
memValue = (memValue & mask) | byteToWrite;
memValue = getValueAsSelectedType(memValue);
const auto currentScanType = magic_enum::enum_name(m_scanType);
if (ImGui::BeginCombo(_("Scan type"), currentScanType.data())) {
for (auto v : magic_enum::enum_values<ScanType>()) {
bool selected = (v == m_scanType);
auto name = magic_enum::enum_name(v);
if (ImGui::Selectable(name.data(), selected)) {
m_scanType = v;
}
if (selected) {
ImGui::SetItemDefaultFocus();
}
}
ImGui::EndCombo();
}

if (!m_addressValuePairs.empty() && ImGui::Button(_("Next scan"))) {
bool InputDisabled = (m_scanType != ScanType::ExactValue && m_scanType != ScanType::GreaterThan &&
m_scanType != ScanType::LessThan);

if (InputDisabled) {
ImGui::BeginDisabled();
}

ImGui::InputScalar(_("Value"), ImGuiDataType_S64, &m_value, NULL, NULL, m_hex ? "%x" : "%i",
m_hex ? ImGuiInputTextFlags_CharsHexadecimal : ImGuiInputTextFlags_CharsDecimal);
m_value = getValueAsSelectedType(m_value);

if (InputDisabled) {
ImGui::EndDisabled();
}

ImGui::Separator();
if (!m_addressValuePairs.empty() && ImGui::Button(_("Next"))) {
auto doesntMatchCriterion = [this, memData, memSize, stride](const AddressValuePair& addressValuePair) {
const uint32_t address = addressValuePair.address;
const int64_t memValue =
getValueAsSelectedType(getMemValue(address, memData, memSize, memBase, stride));

switch (m_scanType) {
case ScanType::ExactValue:
return memValue != m_value;
case ScanType::GreaterThan:
return memValue <= m_value;
case ScanType::LessThan:
return memValue >= m_value;
case ScanType::Changed:
return memValue == addressValuePair.scannedValue;
case ScanType::Unchanged:
return memValue != addressValuePair.scannedValue;
case ScanType::Increased:
return memValue <= addressValuePair.scannedValue;
case ScanType::Decreased:
return memValue >= addressValuePair.scannedValue;
case ScanType::UnknownInitialValue:
return true;
}

return true;
};

std::erase_if(m_addressValuePairs, doesntMatchCriterion);

if (m_addressValuePairs.empty()) {
m_scanType = ScanType::ExactValue;
} else {
for (auto& addressValuePair : m_addressValuePairs) {
addressValuePair.scannedValue = getValueAsSelectedType(
getMemValue(addressValuePair.address, memData, memSize, memBase, stride));
}
}
}

if (!m_addressValuePairs.empty() && ImGui::Button(_("New scan"))) {
m_addressValuePairs.clear();
m_scanType = ScanType::ExactValue;
}

ImGui::Checkbox(_("Hex"), &m_hex);
ImGui::InputScalar(_("Value"), ImGuiDataType_S64, &m_value, NULL, NULL, m_hex ? "%x" : "%i",
m_hex ? ImGuiInputTextFlags_CharsHexadecimal : ImGuiInputTextFlags_CharsDecimal);
m_value = getValueAsSelectedType(m_value);

const auto currentScanValueType = magic_enum::enum_name(m_scanValueType);
if (ImGui::BeginCombo(_("Value type"), currentScanValueType.data())) {
for (auto v : magic_enum::enum_values<ScanValueType>()) {
bool selected = (v == m_scanValueType);
auto name = magic_enum::enum_name(v);
if (ImGui::Selectable(name.data(), selected)) {
m_scanValueType = v;
}
if (selected) {
ImGui::SetItemDefaultFocus();
}
ImGui::Separator();
if (!m_addressValuePairs.empty() && ImGui::Button(_("Freeze all"))) {
for (auto& addressValuePair : m_addressValuePairs) {
addressValuePair.frozen = true;
addressValuePair.frozenValue = getValueAsSelectedType(getMemValue(
addressValuePair.address, memData, memSize, memBase, getStrideFromValueType(m_scanValueType)));
}
ImGui::EndCombo();
}

const auto currentScanType = magic_enum::enum_name(m_scanType);
if (ImGui::BeginCombo(_("Scan type"), currentScanType.data())) {
for (auto v : magic_enum::enum_values<ScanType>()) {
bool selected = (v == m_scanType);
auto name = magic_enum::enum_name(v);
if (ImGui::Selectable(name.data(), selected)) {
m_scanType = v;
}
if (selected) {
ImGui::SetItemDefaultFocus();
}
ImGui::SameLine();
if (!m_addressValuePairs.empty() && ImGui::Button(_("Unfreeze all"))) {
for (auto& addressValuePair : m_addressValuePairs) {
addressValuePair.frozen = false;
}
ImGui::EndCombo();
}

if (!m_hex && stride > 1) {
ImGui::Checkbox(_("Display as fixed-point values"), &m_fixedPoint);
ImGui::SameLine();
if (!m_addressValuePairs.empty() && ImGui::Button(_("Remove frozen addresses"))) {
std::erase_if(m_addressValuePairs, [](const AddressValuePair& pair) { return pair.frozen; });
}

if (ImGui::BeginTable(_("Found values"), 6, tableFlags)) {
if (ImGui::BeginTable(_("Found values"), 7, tableFlags)) {
ImGui::TableSetupColumn(_("Address"));
ImGui::TableSetupColumn(_("Current value"));
ImGui::TableSetupColumn(_("Scanned value"));
ImGui::TableSetupColumn(_("Current"));
ImGui::TableSetupColumn(_("Previous"));
ImGui::TableSetupColumn(_("Freeze"));
ImGui::TableSetupColumn(_("Access"));
ImGui::TableSetupColumn(_("Read breakpoint"));
ImGui::TableSetupColumn(_("Write breakpoint"));
ImGui::TableSetupColumn(_("Add breakpoint"));
ImGui::TableSetupColumn(_("Remove"));
ImGui::TableHeadersRow();

bool as_uint = (m_scanValueType == ScanValueType::Uint);
const auto valueDisplayFormat = m_hex ? "%x"
: (m_fixedPoint && stride > 1) ? (as_uint ? "%u.%u" : "%i.%i")
: (as_uint ? "%u" : "%i");

ImGuiListClipper clipper;
clipper.Begin(m_addressValuePairs.size());
while (clipper.Step()) {
for (int row = clipper.DisplayStart; row < clipper.DisplayEnd; ++row) {
auto& addressValuePair = m_addressValuePairs[row];
const uint32_t currentAddress = addressValuePair.address;
const auto memValue =
getValueAsSelectedType(getMemValue(currentAddress, memData, memSize, memBase, stride));
const auto scannedValue = addressValuePair.scannedValue;
const bool displayAsFixedPoint = !m_hex && m_fixedPoint && stride > 1;

ImGui::TableNextRow();
ImGui::TableSetColumnIndex(0);
ImGui::Text("%x", currentAddress);
ImGui::TableSetColumnIndex(1);
if (displayAsFixedPoint) {
ImGui::Text(valueDisplayFormat, memValue >> 12, memValue & 0xfff);
} else {
ImGui::Text(valueDisplayFormat, memValue);
}
ImGui::SameLine();
auto CheckboxName = fmt::format(f_("Freeze##{}"), row);
if (ImGui::Checkbox(CheckboxName.c_str(), &addressValuePair.frozen)) {
addressValuePair.frozenValue = memValue;
}
ImGui::TableSetColumnIndex(2);
if (displayAsFixedPoint) {
ImGui::Text(valueDisplayFormat, scannedValue >> 12, scannedValue & 0xfff);
} else {
ImGui::Text(valueDisplayFormat, scannedValue);
}
ImGui::TableSetColumnIndex(3);
auto CheckboxName = fmt::format(f_("##{}"), row);
if (ImGui::Checkbox(CheckboxName.c_str(), &addressValuePair.frozen)) {
addressValuePair.frozenValue = memValue;
}
ImGui::TableSetColumnIndex(4);
auto showInMemEditorButtonName = fmt::format(f_("Show in memory editor##{}"), row);
if (ImGui::Button(showInMemEditorButtonName.c_str())) {
const uint32_t editorAddress = currentAddress - memBase;
g_system->m_eventBus->signal(PCSX::Events::GUI::JumpToMemory{editorAddress, stride});
}
ImGui::TableSetColumnIndex(4);
auto addReadBreakpointButtonName = fmt::format(f_("Add read breakpoint##{}"), row);
ImGui::TableSetColumnIndex(5);
auto addReadBreakpointButtonName = fmt::format(f_("Read##{}"), row);
if (ImGui::Button(addReadBreakpointButtonName.c_str())) {
g_emulator->m_debug->addBreakpoint(currentAddress, Debug::BreakpointType::Read, stride,
_("Memory Observer"));
}
ImGui::TableSetColumnIndex(5);
auto addWriteBreakpointButtonName = fmt::format(f_("Add write breakpoint##{}"), row);
ImGui::SameLine();
auto addWriteBreakpointButtonName = fmt::format(f_("Write##{}"), row);
if (ImGui::Button(addWriteBreakpointButtonName.c_str())) {
g_emulator->m_debug->addBreakpoint(currentAddress, Debug::BreakpointType::Write, stride,
_("Memory Observer"));
}
ImGui::TableSetColumnIndex(6);
auto removeButtonName = fmt::format(f_("╳##{}"), row);
if (ImGui::Button(removeButtonName.c_str())) {
m_addressValuePairs.erase(m_addressValuePairs.begin() + row);
clipper.Begin(m_addressValuePairs.size());
break;
}
}
}
ImGui::EndTable();
}

ImGui::EndTabItem();
}

if (ImGui::BeginTabItem(_("Pattern search"))) {
if (m_useSIMD) {
ImGui::TextUnformatted(_("Sequence size: "));
ImGui::SameLine();
ImGui::RadioButton(_("8 bytes (fast)"), &m_sequenceSize, 8);
if (ImGui::RadioButton(_("8 bytes (fast)"), &m_sequenceSize, 8) && strlen(m_sequence) > 8) {
m_sequence[8] = '\0';
}
ImGui::SameLine();
ImGui::RadioButton(_("16 bytes (fast)"), &m_sequenceSize, 16);
if (ImGui::RadioButton(_("16 bytes (fast)"), &m_sequenceSize, 16) && strlen(m_sequence) > 16) {
m_sequence[16] = '\0';
}

Check warning on line 460 in src/gui/widgets/memory_observer.cc

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Complex Method 

PCSX::Widgets::MemoryObserver::draw increases in cyclomatic complexity from 99 to 116, threshold = 9. This function has many conditional statements (e.g. if, for, while), leading to lower code health. Avoid adding more conditionals and code to it without refactoring.

Check warning on line 460 in src/gui/widgets/memory_observer.cc

View check run for this annotation

CodeScene Delta Analysis / CodeScene Code Health Review (main)

❌ Getting worse: Deep, Nested Complexity 

PCSX::Widgets::MemoryObserver::draw increases in nested complexity depth from 7 to 8, threshold = 4. This function contains deeply nested logic such as if statements and/or loops. The deeper the nesting, the lower the code health.
ImGui::SameLine();
ImGui::RadioButton(_("Arbitrary"), &m_sequenceSize, 255);
}
Expand Down
Loading