Closed ghost closed 2 years ago
inactive123
commented
2 years ago @twinaphex if possible, let me review netplay related PRs first, please.
I'm fine with this but I also gave you about a 2 day period to respond to another PR and I got no response there so far. So hopefully feedback time will be quicker from here on out.
Anyway, we can proceed like this.
ghost
commented
2 years ago @twinaphex if possible, let me review netplay related PRs first, please.
I'm fine with this but I also gave you about a 2 day period to respond to another PR and I got no response there so far. So hopefully feedback time will be quicker from here on out.
Anyway, we can proceed like this.
Which PR? I've answered all of them.
ghost
commented
2 years ago Also here is the log from 1.10.0 showing that CRC matching works just fine there:
[INFO] [Lobby]: Searching playlist: D:\RetroArch-Win64\playlists\Nintendo - Super Nintendo Entertainment System.lpl
[INFO] [Lobby]: CRC match 84DA7CFE|crc
[INFO] [Lobby]: Loading core D:\RetroArch-Win64\cores\bsnes_libretro.dll with content file E:\Users\Roberto\Games\SNES\Roms\Contra III The Alien Wars\Contra III - The Alien Wars (USA).sfc
[INFO] [Netplay]: Connecting to 192.168.0.10|55435 (deferred)@Cthulhu-throwaway OK, I don't exactly recall so let's not worry about it for now. But I do recall waiting for a response a few days ago for an issue but I can't exactly recall, so let's move on.
I reverted the PR for now.
trufanov-nok
commented
2 years ago Which PR? I've answered all of them.
Perhaps you're talking about this PR: https://github.com/libretro/libretro-common/pull/195 It's in libretro-common
Description
This merged PR is wrong: https://github.com/libretro/RetroArch/pull/13884 playlist_crc32 does include the |crc suffix.
Source: https://github.com/Cthulhu-throwaway/RetroArch/blob/master/playlist.c#L1921-L1924 As you can see, it's written directly from member crc32 to the json file, which does include the |crc suffix.
I also know it for a fact from my own experience because I've joined rooms with different filenames than mine, but with the same CRC32.
The correct fix is at: https://github.com/libretro/RetroArch/blob/master/tasks/task_netplay_find_content.c#L244 Replacing "%lX|crc" for "%08lX|crc".
Expected behavior
Previous behavior; this PR should be reverted.
Actual behavior
Probably more overhead when comparing crcs.
Version/Commit
https://github.com/libretro/RetroArch/commit/f377c0d04148d37525e98b3047ffbae559f445a5
@twinaphex if possible, let me review netplay related PRs first, please.