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.

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

Jump to bottom

Add shift-click feature on memory view to put focus on pointed address #1083

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

Add shift-click feature on memory view to put focus on pointed address #1083

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8998858
Add shift-click feature on memory view to put focus on pointed address
Vancleeff Apr 22, 2024
2fc324b
Refactor pointer follower feature following review
Vancleeff Apr 26, 2024
8312f48
Update following review
Vancleeff Apr 28, 2024
d2e5216
Removed pointless Mask and adjusted tests
Vancleeff May 3, 2024
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
10 changes: 10 additions & 0 deletions src/ui/viewmodels/MemoryViewerViewModel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "data\context\EmulatorContext.hh"
#include "data\context\GameContext.hh"
#include "data\context\ConsoleContext.hh"

#include "ui\EditorTheme.hh"
#include "ui\viewmodels\WindowManager.hh"
Expand Down Expand Up @@ -895,6 +896,15 @@ void MemoryViewerViewModel::OnClick(int nX, int nY)
}
}

void MemoryViewerViewModel::OnShiftClick(ra::ByteAddress nAddress)
{
const auto& pConsoleContext = ra::services::ServiceLocator::Get<ra::data::context::ConsoleContext>();
const auto nConvertedAddress = pConsoleContext.ByteAddressFromRealAddress(nAddress);
Copy link
Member

Choose a reason for hiding this comment

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

You completely misunderstood my last comment. What you had was fine. I just wanted a test added for clicking on an address that was not convertible.

You can't convert an address just by masking. It may work for PSX, but it wouldn't work for GBA. The real address "$02001234" converts to a RetroAchievements address of "$00009234".

{ 0x008000U, 0x047FFFU, 0x02000000U, RC_MEMORY_TYPE_SYSTEM_RAM, "System RAM" },

The conversion is: subtract 0x02000000, then add 0x008000.

The point of calling ByteAddressFromRealAddress is to let it do the conversion for you. Just provide the raw data and it'll return an RA Address, or 0xFFFFFFFF. I just wanted you to add a test for a case where it returned 0xFFFFFFFF so we could enshrine the intended behavior. The previous behavior would have just jumped to the max address. The current implementation will not jump at all, which I feel is better.

Copy link
Member

Choose a reason for hiding this comment

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

I think I see why you're trying to mask now. The ConsoleContext is initialized from the rcheevos regions, and the PlayStation regions only define the real memory, not the mirrors:

static const rc_memory_region_t _rc_memory_regions_playstation[] = {
    { 0x000000U, 0x00FFFFU, 0x000000U, RC_MEMORY_TYPE_SYSTEM_RAM, "Kernel RAM" },
    { 0x010000U, 0x1FFFFFU, 0x010000U, RC_MEMORY_TYPE_SYSTEM_RAM, "System RAM" }
};

There are mirrors at $80000000 and $A0000000 that should also map to $00000000 (real address), which translates to $00000000 (RetroAchievements address).

This logic should be implemented in ByteAddressFromRealAddress on a per-console basis. Generic masking is not the correct solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I understand how it works, it was a little confusing because of the PSX use-case.
I added these 2 mirrored RAM to consoleinfo.c in rcheevos and now, it works perfectly well on PSX too !
Thanks for your patience @Jamiras !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be sure I did it correctly, here's a commit with the change I made to consoleinfo.c : RetroAchievements/rcheevos@59cd2d2

Copy link
Member

Choose a reason for hiding this comment

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

No, no, no. The regions defined in rcheevos tell emulator developers what memory we expect to be able to find. If you add these mirror regions to the memory map, then they'll become available for writing achievements and appear in search results. See mirror RAM in NES and echo RAM in GameBoy. Not all cores expose the duplicated memory, so achievements relying on it sometimes don't work.

What I meant was something more like this:

ra::ByteAddress ConsoleContext::ByteAddressFromRealAddress(ra::ByteAddress nRealAddress) const noexcept
{
    // Map mirrored memory addresses to non-mirrored addresses.
    // Usually, only the non-mirrored addresses appear in m_vRegions.
    switch (m_nId)
    {
        case RC_CONSOLE_PLAYSTATION:
            if ((nRealAddress & 0xFFE00000) == 0x80000000 || // Kernel and User Memory Mirror (2 Meg) Cached
                (nRealAddress & 0xFFE00000) == 0xA0000000)   // Kernel and User Memory Mirror (2 Meg) Uncached
            {
                nRealAddress &= 0x001FFFFF;
            }
            break;

        case RC_CONSOLE_PLAYSTATION_2:
            if ((nRealAddress & 0xFE000000) == 0x20000000 || // Main RAM, uncached
                (nRealAddress & 0xFE000000) == 0x30000000)   // Main RAM, uncached and accelerated
            {
                nRealAddress &= 0x01FFFFFF;
            }
            break;
    }

    for (const auto& pRegion : m_vRegions)
    {
        if (pRegion.RealAddress < nRealAddress)
        {
            const auto nRegionSize = pRegion.EndAddress - pRegion.StartAddress;
            if (nRealAddress < pRegion.RealAddress + nRegionSize)
                return (nRealAddress - pRegion.RealAddress) + pRegion.StartAddress;
        }
    }

    return 0xFFFFFFFF;
}

if (nConvertedAddress != 0xFFFFFFFF)
nAddress = nConvertedAddress;
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
SetAddress(nAddress);
}

void MemoryViewerViewModel::OnResized(int nWidth, int nHeight)
{
if (s_pFontSurface == nullptr)
Expand Down
1 change: 1 addition & 0 deletions src/ui/viewmodels/MemoryViewerViewModel.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ public:
void SetSize(MemSize value) { SetValue(SizeProperty, ra::etoi(value)); }

void OnClick(int nX, int nY);
void OnShiftClick(ByteAddress address);
void OnResized(int nWidth, int nHeight);
bool OnChar(char c);
void OnGotFocus();
Expand Down
15 changes: 14 additions & 1 deletion src/ui/win32/bindings/MemoryViewerControlBinding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ constexpr UINT WM_USER_INVALIDATE = WM_USER + 1;

INT_PTR CALLBACK MemoryViewerControlBinding::WndProc(HWND hControl, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
const bool bShiftHeld = (GetKeyState(VK_SHIFT) < 0);
switch (uMsg)
{
case WM_PAINT:
Expand All @@ -34,7 +35,10 @@ INT_PTR CALLBACK MemoryViewerControlBinding::WndProc(HWND hControl, UINT uMsg, W
return FALSE;

case WM_LBUTTONUP:
OnClick({ GET_X_LPARAM(lParam) - MEMVIEW_MARGIN, GET_Y_LPARAM(lParam) - MEMVIEW_MARGIN });
if (bShiftHeld)
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
OnShiftClick({GET_X_LPARAM(lParam) - MEMVIEW_MARGIN, GET_Y_LPARAM(lParam) - MEMVIEW_MARGIN});
else
OnClick({ GET_X_LPARAM(lParam) - MEMVIEW_MARGIN, GET_Y_LPARAM(lParam) - MEMVIEW_MARGIN });
return FALSE;

case WM_KEYDOWN:
Expand Down Expand Up @@ -264,6 +268,15 @@ void MemoryViewerControlBinding::OnClick(POINT point)
Invalidate();
}

void MemoryViewerControlBinding::OnShiftClick(POINT point)
{
OnClick(point);
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
const auto& pEmulatorContext = ra::services::ServiceLocator::Get<ra::data::context::EmulatorContext>();
ra::ByteAddress nAddress =
pEmulatorContext.ReadMemory(m_pViewModel.GetAddress(), m_pViewModel.GetSize()) & 0xFFFFFF;
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
m_pViewModel.OnShiftClick(nAddress);
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
}

void MemoryViewerControlBinding::OnGotFocus()
{
m_pViewModel.OnGotFocus();
Expand Down
1 change: 1 addition & 0 deletions src/ui/win32/bindings/MemoryViewerControlBinding.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public:
void ScrollDown();

void OnClick(POINT point);
void OnShiftClick(POINT point);

bool OnKeyDown(UINT nChar);
bool OnEditInput(UINT c);
Expand Down