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

[Vancleeff]

Enhanced user experience by providing a convenient way to navigate to specific pointed memory addresses

[Vancleeff]

Thank you for the review @Jamiras, very helpful !
I made some correction and pushed everything, I'm not sure if the tests are enough for this usecase or if I need to add more console context.

[Vancleeff]

Thanks @Jamiras ! I changed the logic a bit by adding a mask for more complex pointer which are not converted into real addresses when using ByteAddressFromRealAddress.

Example : PSX pointer like 0x80012345 are now masked based on the pConsoleContext.MaxAddress size and then converted with ByteAddressFromRealAddress to point towards 0x12345 address.

I adapted the tests accordingly.

[Jamiras]

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.

[Jamiras]

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.

[Vancleeff]

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 !

[Vancleeff]

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

[Jamiras]

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;
}

[Jamiras]

Are you still working on these? Three of the four PRs have CI failures that need to be addressed, and the fourth has open comments to resolve.

[Vancleeff]

Hello @Jamiras !
Yes, I had to finish my second set, it's now on pending review so I will switch back on these PRs !