Add ram viewer.#2003
Conversation
GPU-accelerated visualization of the PS1's main RAM (2MB/8MB) as a 2048-wide 2D texture with live read/write/execute heatmap overlays. The heatmap stores per-byte cycle timestamps from the CPU debug hooks, uploaded as GL_R32UI textures. The fragment shader computes exponential decay from the current cycle counter, so the visualization is frame-rate independent and never loses access history until overwritten. Features: - Three color-coded channels: read (green), write (red), execute (blue) - Configurable decay half-life - Byte-level grid overlay at high zoom - Hex byte value display using the ImGui font atlas as a glyph texture - Auto-contrast text (white on dark, black on light) - Pan/zoom with shared ZoomableImage base class - Bidirectional integration via event bus: click in RAM viewer jumps hex editor to that address, RAMFocus event zooms RAM viewer to a target address range Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The ImGui font atlas is RGBA32 with glyph shapes in the alpha channel (RGB is white for tinting). The shader was sampling .r instead of .a, producing solid white blocks instead of readable hex digits. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The glyphs were filling 80% of each cell, leaving almost no margin. Reduced to 50% height with proportional width and centered both horizontally and vertically within the cell. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
ImGui's render loop binds pcmd->GetTexID() to GL_TEXTURE_2D before drawing each command. Passing 0 as the texture ID caused ImGui to overwrite the RAM texture binding with the null texture, resulting in all-zero reads in the shader. Pass the actual RAM texture handle so ImGui's bind is consistent with the shader's expectations on texture unit 0. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
When disabled, the base color is treated as black (as if RAM byte = 0), leaving only the heatmap colors visible. Useful for isolating access patterns without the underlying data values competing visually. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
The RAM logger only tracked CPU load/store instructions via debug.cc::process(). DMA transfers (GPU, SPU, CD-ROM, MDEC, etc.) bypass the CPU instruction path and go through checkDMAread/ checkDMAwrite instead. These now feed into the RAM logger so DMA activity is visible in the heatmap. Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
Completes the extraction started when ZoomableImage was created. VRAMViewer now shares the zoom/pan state members and zoom() method with RAMViewer via the common base class. VRAMViewer keeps its own moveTo() (VRAM-specific UV-space transform), resetView() override (adds magnifier reset), and the magnifier-aware scroll handling in drawVRAM (ctrl+scroll adjusts magnifier instead of zoom). Signed-off-by: Nicolas 'Pixel' Noble <nicolas@nobis-crew.org>
📝 WalkthroughWalkthroughAdds a RAM logging system that records per-byte read/write/execute timestamps, GPU-backed heatmaps, and a shader-driven RAM viewer widget; integrates logging into CPU/DMA/debug paths, wires a RAMLogger into the Emulator, and adds shared zoom/pan infrastructure for viewers. Changes
Sequence DiagramsequenceDiagram
participant CPU as CPU
participant Debug as Debug
participant Emulator as Emulator
participant Logger as RAMLogger
participant GPU as OpenGL
participant GUI as GUI
participant Viewer as RAMViewer
CPU->>Debug: execute instruction / load/store / DMA
Debug->>Logger: recordAccess(physAddr, width, type, cycle)
Logger->>Logger: store cycle timestamps (per-byte)
Viewer->>GUI: draw() / visible
GUI->>Emulator: ensure m_ramLogger exists
Emulator->>Logger: enable()
Logger->>GPU: create textures (RAM, heatmaps)
GUI->>Logger: uploadRAM()
Logger->>GPU: glTexSubImage2D(RAM bytes)
GUI->>Logger: uploadHeatmaps()
Logger->>GPU: glTexSubImage2D(heatmap arrays)
Viewer->>GPU: bind textures & set uniforms
GPU-->>Viewer: render sampled RAM + heatmaps
Viewer-->>GUI: hover/click → emit RAMFocus / JumpToMemory
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/debug.cc (1)
228-241: Nested ternaries for width could be clearer.The width calculations work correctly, but the nested ternary expressions reduce readability. Consider extracting to a helper or using a more explicit form.
♻️ Optional: Clearer width calculation
// RAM logger: record loads and stores if (ramLoggerEnabled) { PSXAddress loadStoreAddr(normalizeAddress(offset)); if (loadStoreAddr.type == PSXAddress::Type::RAM) { if (isLoad) { - unsigned width = (isLB || isLBU) ? 1 : (isLH || isLHU) ? 2 : 4; + unsigned width = 4; + if (isLB || isLBU) width = 1; + else if (isLH || isLHU) width = 2; ramLogger->recordAccess(loadStoreAddr.physical, width, RAMLogger::AccessType::Read, cycle); } if (isStore) { - unsigned width = isSB ? 1 : isSH ? 2 : 4; + unsigned width = 4; + if (isSB) width = 1; + else if (isSH) width = 2; ramLogger->recordAccess(loadStoreAddr.physical, width, RAMLogger::AccessType::Write, cycle); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/debug.cc` around lines 228 - 241, The nested ternary expressions for computing access width reduce readability in the RAM logging block; replace them with a small helper (e.g., computeLoadWidth/computeStoreWidth or a single computeAccessWidth) and call it where ramLogger->recordAccess is invoked inside the ramLoggerEnabled branch; reference the existing symbols PSXAddress, normalizeAddress, isLoad, isLB, isLBU, isLH, isLHU, isStore, isSB, isSH and ensure the new helper returns unsigned width used by ramLogger->recordAccess with RAMLogger::AccessType::Read/Write and cycle.src/core/ramlogger.h (2)
64-66: Note: Significant memory footprint.The three timestamp arrays consume 96MB total (3 × 8MB × 4 bytes). This is inherent to the per-byte tracking design. Consider documenting this memory requirement or lazily allocating these arrays only when the logger is first enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ramlogger.h` around lines 64 - 66, The three fixed-size arrays m_readTimestamps, m_writeTimestamps, and m_execTimestamps allocate ~96MB at static construction causing a large memory footprint; change them from static arrays to lazily allocated containers (e.g. std::vector<uint32_t> or std::unique_ptr<uint32_t[]>) initialized empty and allocate/reserve c_maxBytes only when the RAM logger is first enabled (e.g. in RamLogger::enable or initialize method), and free or clear them on disable; alternatively, if you prefer not to change allocation strategy, add a clear comment above these members documenting the ~96MB requirement so callers are aware.
22-23: Prefer<cstring>over<string.h>for C++.While both work,
<cstring>is the C++ header and placesmemsetin thestdnamespace.Suggested fix
`#include` <stdint.h> -#include <string.h> +#include <cstdint> +#include <cstring>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ramlogger.h` around lines 22 - 23, Replace the C header include "<string.h>" with the C++ header "<cstring>" in the includes at the top of ramlogger.h and update any uses of C string functions (e.g., memset) in functions or methods within the file (look for memset calls) to their std:: namespace equivalents (e.g., std::memset) so the code uses the C++ standard headers/namespaces consistently.src/core/ramlogger.cc (1)
40-75: Potential resource leak on partial initialization failure.If texture creation partially succeeds (e.g., read and write heatmaps created but exec fails), the early return leaves textures allocated but
m_hasResourcesremains false. A subsequentenable()call would attempt to recreate all textures without first cleaning up the existing ones.Consider either cleaning up on failure or checking
exists()beforecreate().Suggested defensive approach
+ // Clean up any partial state first + if (m_readHeatmapTex.exists()) m_readHeatmapTex.destroy(); + if (m_writeHeatmapTex.exists()) m_writeHeatmapTex.destroy(); + if (m_execHeatmapTex.exists()) m_execHeatmapTex.destroy(); + if (m_ramTexture.exists()) m_ramTexture.destroy(); + // Create heatmap textures (GL_R32UI - unsigned integer) m_readHeatmapTex.create(c_width, c_maxHeight, GL_R32UI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/ramlogger.cc` around lines 40 - 75, enable() can leave partially-created OpenGL textures allocated on early return, causing resource leaks because m_hasResources stays false; modify enable() to either check texture exists() before create() or free any textures created on failure. Specifically, after each .create(...) and exists() check for m_readHeatmapTex, m_writeHeatmapTex, m_execHeatmapTex and m_ramTexture, ensure that on failure you call the corresponding cleanup/destroy method (or a helper like releaseResources()) to delete any previously-created textures and reset state, or change flow to skip create() if exists() is true so repeated enable() calls are idempotent; update m_hasResources only after all resources are successfully created.src/gui/widgets/vram-viewer.cc (1)
726-735:moveTo()shadows the base class method with different semantics.This
VRAMViewer::moveTo()takes UV-space coordinates and translates them differently thanZoomableImage::moveTo(), which takes pixel positions. While this works, it could cause confusion since the base class method is hidden. Consider renaming this to something more descriptive likemoveToUV()orcenterOnVRAMPos()to make the semantic difference explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/vram-viewer.cc` around lines 726 - 735, The VRAMViewer::moveTo method uses UV-space semantics that differ from ZoomableImage::moveTo (pixel-space); rename VRAMViewer::moveTo to a descriptive name like moveToUV or centerOnVRAMPos, update its declaration in the class and all call sites to the new name, and remove/hide any accidental override so the base-class ZoomableImage::moveTo remains unshadowed; ensure any tests or references (e.g., calls in UI code or signal handlers) are updated to call the new method name and keep the original implementation body unchanged.src/gui/widgets/ram-viewer.h (1)
22-22: Unused<string>include.The
<string>header doesn't appear to be used directly in this header file. Consider removing it if not needed.Suggested fix
`#pragma` once -#include <string> - `#include` "GL/gl3w.h"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gui/widgets/ram-viewer.h` at line 22, The header includes an unused <string> header; remove the `#include` <string> line from src/gui/widgets/ram-viewer.h and rebuild, and if any declarations in the RamViewer class (or related symbols in this header) rely on std::string move that dependency to the implementation file (ram-viewer.cpp) or forward-declare as needed so the header no longer requires <string>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/ram-viewer.cc`:
- Around line 209-214: In PCSX::Widgets::RAMViewer::focusOn, the address is
masked with 0x1fffff unconditionally then optionally with 0x7fffff, which
truncates addresses for 8MB mode; change the logic so the mask is chosen based
on Emulator::Setting8MB (apply 0x7fffff when
g_emulator->settings.get<Emulator::Setting8MB>() is true, otherwise apply
0x1fffff) so the address is correctly converted to a physical RAM offset.
---
Nitpick comments:
In `@src/core/debug.cc`:
- Around line 228-241: The nested ternary expressions for computing access width
reduce readability in the RAM logging block; replace them with a small helper
(e.g., computeLoadWidth/computeStoreWidth or a single computeAccessWidth) and
call it where ramLogger->recordAccess is invoked inside the ramLoggerEnabled
branch; reference the existing symbols PSXAddress, normalizeAddress, isLoad,
isLB, isLBU, isLH, isLHU, isStore, isSB, isSH and ensure the new helper returns
unsigned width used by ramLogger->recordAccess with
RAMLogger::AccessType::Read/Write and cycle.
In `@src/core/ramlogger.cc`:
- Around line 40-75: enable() can leave partially-created OpenGL textures
allocated on early return, causing resource leaks because m_hasResources stays
false; modify enable() to either check texture exists() before create() or free
any textures created on failure. Specifically, after each .create(...) and
exists() check for m_readHeatmapTex, m_writeHeatmapTex, m_execHeatmapTex and
m_ramTexture, ensure that on failure you call the corresponding cleanup/destroy
method (or a helper like releaseResources()) to delete any previously-created
textures and reset state, or change flow to skip create() if exists() is true so
repeated enable() calls are idempotent; update m_hasResources only after all
resources are successfully created.
In `@src/core/ramlogger.h`:
- Around line 64-66: The three fixed-size arrays m_readTimestamps,
m_writeTimestamps, and m_execTimestamps allocate ~96MB at static construction
causing a large memory footprint; change them from static arrays to lazily
allocated containers (e.g. std::vector<uint32_t> or std::unique_ptr<uint32_t[]>)
initialized empty and allocate/reserve c_maxBytes only when the RAM logger is
first enabled (e.g. in RamLogger::enable or initialize method), and free or
clear them on disable; alternatively, if you prefer not to change allocation
strategy, add a clear comment above these members documenting the ~96MB
requirement so callers are aware.
- Around line 22-23: Replace the C header include "<string.h>" with the C++
header "<cstring>" in the includes at the top of ramlogger.h and update any uses
of C string functions (e.g., memset) in functions or methods within the file
(look for memset calls) to their std:: namespace equivalents (e.g., std::memset)
so the code uses the C++ standard headers/namespaces consistently.
In `@src/gui/widgets/ram-viewer.h`:
- Line 22: The header includes an unused <string> header; remove the `#include`
<string> line from src/gui/widgets/ram-viewer.h and rebuild, and if any
declarations in the RamViewer class (or related symbols in this header) rely on
std::string move that dependency to the implementation file (ram-viewer.cpp) or
forward-declare as needed so the header no longer requires <string>.
In `@src/gui/widgets/vram-viewer.cc`:
- Around line 726-735: The VRAMViewer::moveTo method uses UV-space semantics
that differ from ZoomableImage::moveTo (pixel-space); rename VRAMViewer::moveTo
to a descriptive name like moveToUV or centerOnVRAMPos, update its declaration
in the class and all call sites to the new name, and remove/hide any accidental
override so the base-class ZoomableImage::moveTo remains unshadowed; ensure any
tests or references (e.g., calls in UI code or signal handlers) are updated to
call the new method name and keep the original implementation body unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a28c4711-8395-421e-87e4-70f30cab92a7
📒 Files selected for processing (15)
src/core/debug.ccsrc/core/debug.hsrc/core/psxemulator.ccsrc/core/psxemulator.hsrc/core/ramlogger.ccsrc/core/ramlogger.hsrc/core/system.hsrc/gui/gui.ccsrc/gui/gui.hsrc/gui/widgets/ram-viewer.ccsrc/gui/widgets/ram-viewer.hsrc/gui/widgets/vram-viewer.ccsrc/gui/widgets/vram-viewer.hsrc/gui/widgets/zoomable-image.ccsrc/gui/widgets/zoomable-image.h
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gui/widgets/ram-viewer.cc`:
- Around line 218-223: The computed endAddr can overflow when doing uint32_t
endAddr = address + size; — clamp/saturate endAddr to the RAM size limit before
converting to coordinates: compute endAddr = std::min(address + size, RAM_SIZE)
(or equivalent saturating logic) and then compute x1, y1, x2, y2 from address
and the clamped endAddr so the focus rectangle never wraps; update the logic
around the existing endAddr/address/size -> x1,y1,x2,y2 conversion to use the
clamped value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70457977-8156-44b1-9670-93c283af8b1f
📒 Files selected for processing (1)
src/gui/widgets/ram-viewer.cc
| uint32_t endAddr = address + size; | ||
| float x1 = float(address % 2048); | ||
| float y1 = float(address / 2048); | ||
| float x2 = float(endAddr % 2048); | ||
| float y2 = float(endAddr / 2048); | ||
|
|
There was a problem hiding this comment.
Clamp endAddr to avoid overflow/wrap in focus range math.
uint32_t endAddr = address + size; can wrap and generate an invalid focus rectangle for large size. Saturate to RAM size before converting to coordinates.
Proposed fix
void PCSX::Widgets::RAMViewer::focusOn(uint32_t address, uint32_t size) {
// Convert PS1 address to physical RAM offset
- if (g_emulator->settings.get<Emulator::Setting8MB>()) {
+ bool is8MB = g_emulator->settings.get<Emulator::Setting8MB>();
+ if (is8MB) {
address &= 0x7fffff;
} else {
address &= 0x1fffff;
}
// Convert to pixel coordinates in the 2048-wide layout
- uint32_t endAddr = address + size;
+ const uint32_t ramSize = is8MB ? 0x800000u : 0x200000u;
+ uint32_t endAddr = (size > (ramSize - address)) ? ramSize : (address + size);
float x1 = float(address % 2048);
float y1 = float(address / 2048);
float x2 = float(endAddr % 2048);
float y2 = float(endAddr / 2048);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gui/widgets/ram-viewer.cc` around lines 218 - 223, The computed endAddr
can overflow when doing uint32_t endAddr = address + size; — clamp/saturate
endAddr to the RAM size limit before converting to coordinates: compute endAddr
= std::min(address + size, RAM_SIZE) (or equivalent saturating logic) and then
compute x1, y1, x2, y2 from address and the clamped endAddr so the focus
rectangle never wraps; update the logic around the existing endAddr/address/size
-> x1,y1,x2,y2 conversion to use the clamped value.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
vsprojects/gui/gui.vcxproj.filters (1)
112-113: Assign Visual Studio filters for the newly added widget files.These entries are currently unfiltered, so they’ll appear outside the existing widget groups in Solution Explorer.
📁 Proposed filter mapping
- <ClCompile Include="..\..\src\gui\widgets\zoomable-image.cc" /> - <ClCompile Include="..\..\src\gui\widgets\ram-viewer.cc" /> + <ClCompile Include="..\..\src\gui\widgets\zoomable-image.cc"> + <Filter>Source Files\widgets</Filter> + </ClCompile> + <ClCompile Include="..\..\src\gui\widgets\ram-viewer.cc"> + <Filter>Source Files\widgets</Filter> + </ClCompile> ... - <ClInclude Include="..\..\src\gui\widgets\zoomable-image.h" /> - <ClInclude Include="..\..\src\gui\widgets\ram-viewer.h" /> + <ClInclude Include="..\..\src\gui\widgets\zoomable-image.h"> + <Filter>Header Files\widgets</Filter> + </ClInclude> + <ClInclude Include="..\..\src\gui\widgets\ram-viewer.h"> + <Filter>Header Files\widgets</Filter> + </ClInclude>Also applies to: 215-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vsprojects/gui/gui.vcxproj.filters` around lines 112 - 113, The two new source entries (zoomable-image.cc and ram-viewer.cc) were added to the project filters but not assigned to a filter, causing them to appear outside the Widgets group; update the gui.vcxproj.filters so each <ClCompile Include="..\..\src\gui\widgets\zoomable-image.cc"> and <ClCompile Include="..\..\src\gui\widgets\ram-viewer.cc"> has an associated <Filter> element that maps them into the existing widget filter (e.g., the same filter string used by other widget files), and apply the same change for the duplicate entries at 215-216 so both files appear under the correct Widgets filter in Solution Explorer.vsprojects/core/core.vcxproj.filters (1)
148-148: Assign explicit Visual Studio filters for the new RAM logger files.Line 148 and Line 301 omit
<Filter>while most entries in this file set one explicitly. It won’t break builds, but it makes Solution Explorer grouping inconsistent.🧩 Suggested tidy-up
- <ClCompile Include="..\..\src\core\ramlogger.cc" /> + <ClCompile Include="..\..\src\core\ramlogger.cc"> + <Filter>Source Files</Filter> + </ClCompile> ... - <ClInclude Include="..\..\src\core\ramlogger.h" /> + <ClInclude Include="..\..\src\core\ramlogger.h"> + <Filter>Header Files</Filter> + </ClInclude>Also applies to: 301-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vsprojects/core/core.vcxproj.filters` at line 148, The ClCompile entries for the new ramlogger.cc lack an explicit <Filter>, causing inconsistent Solution Explorer grouping; locate the ClCompile element with Include="..\..\src\core\ramlogger.cc" and add a <Filter> element that matches the existing Core source-file filter used by other core .cc entries (use the exact filter string used elsewhere to keep grouping consistent); apply the same change to both occurrences of the ramlogger.cc ClCompile entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@vsprojects/core/core.vcxproj.filters`:
- Line 148: The ClCompile entries for the new ramlogger.cc lack an explicit
<Filter>, causing inconsistent Solution Explorer grouping; locate the ClCompile
element with Include="..\..\src\core\ramlogger.cc" and add a <Filter> element
that matches the existing Core source-file filter used by other core .cc entries
(use the exact filter string used elsewhere to keep grouping consistent); apply
the same change to both occurrences of the ramlogger.cc ClCompile entry.
In `@vsprojects/gui/gui.vcxproj.filters`:
- Around line 112-113: The two new source entries (zoomable-image.cc and
ram-viewer.cc) were added to the project filters but not assigned to a filter,
causing them to appear outside the Widgets group; update the gui.vcxproj.filters
so each <ClCompile Include="..\..\src\gui\widgets\zoomable-image.cc"> and
<ClCompile Include="..\..\src\gui\widgets\ram-viewer.cc"> has an associated
<Filter> element that maps them into the existing widget filter (e.g., the same
filter string used by other widget files), and apply the same change for the
duplicate entries at 215-216 so both files appear under the correct Widgets
filter in Solution Explorer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8738b9eb-7b4a-477b-a6b9-a84114a29084
📒 Files selected for processing (4)
vsprojects/core/core.vcxprojvsprojects/core/core.vcxproj.filtersvsprojects/gui/gui.vcxprojvsprojects/gui/gui.vcxproj.filters
✅ Files skipped from review due to trivial changes (1)
- vsprojects/core/core.vcxproj
No description provided.