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.

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

PCSX2: Save/load slot improvements #2574

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
4 participants
@arcum42
Copy link
Contributor

commented Sep 3, 2018

I figured this one was enough to do a PR request on rather than committing.

With these changes, saveslots will be labeled as either empty or with the date that the file was last updated on. The menu items for loading them are also disabled if the slot is empty.

It's possible if you are very fast to access the menu before the slots change. It updates when the crc changes. When you save a saveslot, the menu item also changes to show the time you told it to save until the file is actually done saving.

Finding times for it to update the ui was a bit tricky. This was tested with Linux, but not Windows, and was tested with Isos, but not actual disks (no dvd drive on the computer I was testing with).

Testing, fixes and improvements are welcome, and if there's anything I should do differently next time I do a PR, let me know...

Close #2073

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

I honestly prefer tabs over spaces. When trying to write a new line the automatic indentation is a tab. And you might forget and leave it like that without realizing that the file uses spaces instead of tabs.
It's easier to maintain.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

Most of the reformatting was simply that I ran clang-format on the main files I was working on while I was working on them. As such, nothing really conscious on spaces vs. tabs there. In theory it should've gone to the preferred format for the project...

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

I may go through and rerun it, though, as it's a bit odd that that's inconsistent on tabs vs. spaces...

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

Okay, yeah, looking into it, the last line in this file says "Use-tab: never":
https://github.com/PCSX2/pcsx2/blob/master/.clang-format

That causes it to change all the tabs to spaces when I run clang-format. I knew I typed tabs when I was coding...

@lightningterror lightningterror changed the title Saveslot improvements PCSX2: Saveslot improvements Sep 3, 2018

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 3, 2018

I just tested it and it works nicely, great work 👍

@lightningterror lightningterror requested a review from turtleli Sep 3, 2018

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2018

Thanks!

You don't want to know how many places I tried putting the UI update code before I got it working consistently. Making sure it updated when you switched isos, when you load a saveslot, and on both normal and fast boot was surprisingly tricky. For a while I was banging my head against a wall, because it worked perfectly except if you just started up pcsx2 and did a fast boot of whatever iso was already chosen.

Not to mention my first attempt at it ended up being thrown out and totally redone. My first thought was "well, I'll just rescan the files when you tell it to save a saveslot". The new file isn't written yet at that point...

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Are you fine if I do a rebase/squash and clean up the tabs/spaces ?

@lightningterror lightningterror requested a review from ramapcsx2 Sep 4, 2018

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

I'm fine with both of those. I tend to think the defaults should be changed on clang-format in pcsx2 to use tabs, anyways.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

I tend to think the defaults should be changed on clang-format in pcsx2 to use tabs, anyways.

Neither is the correct answer honestly since there are files that use tabs and there are files that use spaces 😄

@lightningterror lightningterror force-pushed the saveslots branch from dd54d0e to 77271ed Sep 4, 2018

@lightningterror lightningterror changed the title PCSX2: Saveslot improvements PCSX2: Save/load slot improvements Sep 4, 2018

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

As witness that there's also a file right next to it that tells Linux editors that tabs should be used.

@lightningterror lightningterror force-pushed the saveslots branch from 77271ed to da0e33c Sep 4, 2018

else
{
return wxInvalidDateTime;
}

This comment has been minimized.

Copy link
@lightningterror

lightningterror Sep 4, 2018

Member

Maybe the brackets can be removed on the if and else or do the reverse and add where there aren't any, not sure which is better.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

I tested a bit more, it works for physical discs as well.

It doesn't work for the bios tho but I dunno if that's important or not :)
A solution would be to use the default crc 0x00000000 (bios) and then do the switch when a game crc is selected.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Yeah, given the slot test explicitly says to return as false if the crc is 0, and I'm pretty sure the file name has the crc in it, I'm not surprised doing save slots in the bios doesn't work. I'm not really sure if that's an issue, either, as I'm not sure why you'd want to save to a slot when you were in the bios.

I'm glad to hear physical discs work, though. I thought they would, but didn't have a way to check.

It basically updates when the crc updates, so swapping a disc ought to be fine...

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Ye swapping works fine and nobody really cares about the bios so I'm fine with how it works atm 😃

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Saveslots.h isn't included in VS project files.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Ah, right. I did add that header in. I can try to add it to the visual studio project in a text editor, but if anyone feels like actually adding it in with Visual Studio, it'd be appreciated. ^_^

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

If anyone feels like actually adding it in with Visual Studio, it'd be appreciated.

Yeah I also don't know how yet so used normal text editor as well, not sure if I did it properly xD

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

To be totally fair, I do have a Windows computer I probably could've used for this as well. I don't have the branch checked out on there, though, and I've been doing most of the pcsx2 development over on the Linux computer.

Doing pcsx2 development on a computer with an ancient Logitech controller and a GT640 graphics card when I could be doing it on one with a steam controller and a GTX960 is probably weird, but I'm rather more comfortable on the Linux coding side...

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

Well, I just did a last test or two, and it seems like it's fine, so we're probably all right with it being merged at this point. It's possible something might turn up later, but we can always fix it at that point, if it comes to that.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

Actually I did notice an issue just now, while I agree we don't need the feature on the bios it does introduce some regressions.

If I'm on the bios I can no longer use the Load State menu since the options are greyed out.
Maybe the grey out menu code can be activated once the game disc is detected.

Likewise I would also suggest making the dates and Empty tag names in the slots function only when a disc is detected. Doing this fixes 2 issues. Savedates shouldn't appear on the bios because if you do a reboot again they appear named as Empty when in reality they're not.

Short answer the slots should work the old way like they do on master while the bios is active only.

Edit: There's also an issue with Backup save being greyed out the first time you boot the game even if there is a backup already present.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

Alright, I commented out the code in the check to see if a slot was empty that automatically said it was empty if you were in the bios.

That code was in there previously, incidentally. It's just that no one ever bothered to call the check to see if the slot was empty anywhere in the code before...

#pragma once

#ifndef __SAVESLOTS_H__
#define __SAVESLOTS_H__

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 5, 2018

Member

Double underscore is reserved by the C++ standard for implementation use.

This comment has been minimized.

Copy link
@lightningterror

lightningterror Sep 5, 2018

Member

Is __SAVESLOTS_H__ even needed here ? I don't see anywhere else that it's used.

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 5, 2018

Member

It's an include guard.

This comment has been minimized.

Copy link
@gregory38

gregory38 Sep 6, 2018

Contributor

which is useless with #pragma once


~Saveslot() {}

__forceinline bool isUsed()

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 5, 2018

Member

Are the forceinlines necessary?

Show resolved Hide resolved pcsx2/gui/UpdateUI.cpp
bool SaveStateBase::isSlotUsed(int slot)
{
if (ElfCRC == 0)
return false;

This comment has been minimized.

Copy link
@lightningterror

lightningterror Sep 5, 2018

Member

What about this check ? Is this fine being enabled ?

}
};

void Sstates_updateLoadBackupMenuItem( bool isBeforeSave = false);
void Saveslots_UpdateFromDisk()

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 5, 2018

Member

This function seems to be unused?

@@ -113,6 +113,8 @@ class SaveStateBase
virtual ~SaveStateBase() { }

static wxString GetFilename( int slot );
static bool isSlotUsed(int slot);
static wxDateTime GetSlotTimestamp(int slot);

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 5, 2018

Member

I don't see these functions being in use either.

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@arcum42 project formatting was a mess. So I (we) decided to use clang-format to keep same format everywhere. However I haven't converted all the code yet. So some parts still use tabs. Tab isn't nice because it doesn't look the same on various places. Git tab versus github tab versus your editor setting.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@gregory It's easier to get the number of spaces wrong on indentation then the number of tabs, though, and if you run a file through clang-format, then use an editor that uses the editor settings in the .editorconfig file in this project, the editor will then use tabs on anything you add, making the formatting mixed.

I also don't think there's a general consensus on the team on this, since the very first thing I was asked to do on this PR was change the spaces clang-format put in back to tabs...

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

@lightningterror Regarding the backup slot, I'm just carrying over the already broken functionality. In the main branch, I just did several save states, it changed the menu item to say there was a backup, and the backup never generated and it never enabled.

If anything, I think the backup slot has worked more often for me in this branch.

I can do some things to try to see if I can get it to work better, since right now it's only calling it in exactly the same spots the original did. It's not a regression if that doesn't work, though...

Also, how is it supposed to act if you have more than one backup file in the folder? Right now, it's whatever slot you last used or have selected by the keyboard. I'd assume that'd be slot 0 at start.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Yeah it's broken on master as well, I didn't notice it before so that's why I mentioned it and thought it was a regression.

How it should work is when you first boot the emu with it's game without pressing any load/saves slot0 shouldn't be greyed out and it should show Slot 0 instead of just Slot if a backup slot is available.
Thing is the hotkey for loading a backup works.

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2018

Yeah, the hotkeys totally bypass the logic for whether anything should be enabled or not. That's one reason the saves and loads are more complicated now, too. Before it just tried to save or load the slot, and if it failed, it failed. Offering options that aren't actually available isn't generally a good idea, though.

That last commit I just did adds a check for the backup slot in the same place that's called every time a game loads, though. I was able to load a backup file after starting an iso right after starting the emulator with that.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

Nice work, it works as it should now.

@turtleli is the PR fine for a merge ?

@arcum42

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2018

Thanks. And hopefully so.

The main things I could see left are that I left a function for logging in and some commented out logging code (but those could still be useful for troubleshooting down the line), and I left in an unused variable in the class. It's a variable I'd probably be using if I go back and add more improvements to the save slot code at some point, though.

I personally think it's fine for a commit as is. (And I'd really like to see it in, because it's so much easier to tell if a saveslot is used or not...)

@turtleli
Copy link
Member

left a comment

It would be nice if changes that only affect whitespace were not included in the PR. It increases the diff and makes reviewing more difficult.

else
return wxFileExists( SaveStateBase::GetFilename( num ) );
}
Saveslot saveslot_cache[10];

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Maybe initialise the saveslots here instead of in MakeStatesSubMenu?

Also, a g_ prefix would make it obvious that it's a global variable.

{
return wxsFormat(_("Slot %d - Empty"), slot_num);
}
else

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

else after return is redundant.

else
{
return wxInvalidDateTime;
}

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Ditto.

crc = ElfCRC;
}

~Saveslot() {}

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Maybe remove this?

Show resolved Hide resolved pcsx2/gui/Saveslots.h
u32 slot_num;
bool empty;
wxDateTime updated;
wxString note;

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

I'd prefer this to be removed since it's unused right now.


class Saveslot
{
protected:

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Label is unused.

@@ -1212,6 +1212,9 @@
<ClInclude Include="..\..\gui\AppGameDatabase.h">
<Filter>AppHost</Filter>
</ClInclude>
<ClInclude Include="..\..\gui\Saveslots.h">
<Filter>AppHost</Filter>

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Maybe this should go in AppHost\Include instead.

This comment has been minimized.

Copy link
@arcum42

arcum42 Sep 10, 2018

Author Contributor

I have no idea, as all of my changes have been done from Linux, and adding the file to Visual Studio was taken care of by lightningterror. Feel free to change it if you want to.

This comment has been minimized.

Copy link
@lightningterror

lightningterror Sep 10, 2018

Member

My bad, I was following the cpp file example.


//_SaveLoadStuff( true );
_SaveLoadStuff( SysHasValidState() );
_SaveLoadStuff(SysHasValidState());
//Console.WriteLn("UI_UpdateSysControls done!");

This comment has been minimized.

Copy link
@turtleli

turtleli Sep 9, 2018

Member

Are the debug logging lines in this function still needed?

PCSX2: Save/load slot improvements.
With these changes, saveslots will be labeled as either empty or
with the date that the file was last updated on. The menu items
for loading them are also disabled if the slot is empty.

It's possible if you are very fast to access the menu before the
slots change. It updates when the crc changes. When you save a saveslot,
the menu item also changes to show the time you told it to save until the
file is actually done saving.

Also fix an issue with backup saveslots not working properly from the
gui on first load.
@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Tab isn't nice because it doesn't look the same on various places. Git tab versus github tab versus your editor setting.

It's a good idea they look different on github, that way we can spot the differences if someone uses different formatting methods than what the file is using. On VS and Notepad++ you don't see it unless you actually check the spacing yourself.

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

It's easier to get the number of spaces wrong on indentation then the number of tabs, though, and if you run a file through clang-format, then use an editor that uses the editor settings in the .editorconfig file in this project, the editor will then use tabs on anything you add, making the formatting mixed.

I also don't think there's a general consensus on the team on this, since the very first thing I was asked to do on this PR was change the spaces clang-format put in back to tabs...

Any sane editor should replace tab by the good amount of space. Otherwise clang-format will do it for you. So leading spaces aren't difficult to manage. However it is more annoying, if your tabs are 4 space equivalent in your text editor but 8 space equivalent on (for example) git diff/github.

The issue of .editorconfig is that we should finish to convert the full git to clang-format (whatever is space or tab).

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

This might make things a bit more difficult for new contributors for those that don't use clang format.
Well even if you don't use clang it's easy to do a reformat on a file (1tab=4spaces).

Anyway I did went through and cleaned up the rest of the glsl shaders to use spaces so I could start from there with the fx shaders as well.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

@turtleli can I merge this ?

@turtleli

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

On VS and Notepad++ you don't see it unless you actually check the spacing yourself.

VS has a view whitespace option. Dunno about Notepad++ (I don't use it).

This might make things a bit more difficult for new contributors for those that don't use clang format.

VS2017 has built-in clang-format support (no plugin necessary). That'll make things easier for Windows users at least.

@turtleli

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

can I merge this ?

You can merge if you want.

My complaints are:

  1. There are unrelated formatting changes - it makes it harder to see what's actually changing.
  2. I think the member init list change should have been done.
@lightningterror

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Ok I'll merge it, the robustness can be improved later, as for the formatting it's not that big so it should be ok, and would be a PITA to readjust the code cause of that.

@lightningterror lightningterror merged commit 9cb35a8 into master Sep 10, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lightningterror lightningterror deleted the saveslots branch Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.