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

Quicksave #1070

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Quicksave #1070

wants to merge 12 commits into from

Conversation

mymikemiller
Copy link

Adds a quicksave feature in the form of a save icon on the top right side of the screen. Tapping this icon overwrites your most recent manual save with a new save.

Request for feedback:
How do you feel about this implementation?

@mymikemiller
Copy link
Author

These checks are failing due to reasons other than my change.

The first one fails due to a timeout of 45 minutes (!) while building RealmSwift. Another PR is able to get a bit farther than RealmSwift before timing out, but not much)

The second fails due to an error in Microsoft's Xcode Build Task which may be the same problem as this similar open issue from 9 days ago.

Both checks fail for the same reasons in this other PR.

Please advise :)

@JoeMatt
Copy link
Member

JoeMatt commented Jan 31, 2019

@mymikemiller Don't worry about the checks, all the CIs are breaking for their own reasons.

I just haven't' had time to dig into this PR.
Maybe if you could attach a screen shot or video in use you could get some quicker feedback.

@JoeMatt JoeMatt self-assigned this Jan 31, 2019
@JoeMatt JoeMatt added the requires feedback pending unanswered question label Jan 31, 2019
@JoeMatt
Copy link
Member

JoeMatt commented Jan 31, 2019

Rather than an always visible button, I think something less obtrusive and quicker to access would be preferred.

All the space that isn't an on-screen control could be used for gestures. Maybe something like a 3 finger double tap. Or twice finger drag from outer edge towards each other over 150ish pixels, which could be used by quickly moving your thumbers off the on screen controls and dragging them from the edge towards the middle, then you wouldn't have to even adjust your grip for standard holding styles.

Also, looks like the behaviour is to delete the last manual save. I think that's too likely to delete something someone wanted to keep. It would be better to just not delete any previous saves. We only ever auto delete saves with autosaves, which are specifically flagged as such, and there's some other conditions such as time interval since last au

And for the button layout, it should use anchors instead of calculating pixel offsets to reduce the likely hood of layout bugs if the layout function isn't specifically called on events like frame changes due to picture in picture, incoming call banners, rotation etc.

@sevdestruct What you think?

@sevdestruct
Copy link
Collaborator

sevdestruct commented Feb 2, 2019

I haven't taken a look at this yet, but…

Always visible button…

I agree: might not be best as an always visible button in its current execution. I've had something in mind for UI 2.0 for a new custom touch pause menu/UI and how to get quicksave and fast forward and such to be quickly accessible, but haven't drafted designs for it yet…

Gestures…

Update: Edge swipe quick action bar UI option would work, and good solution for now that we can put other stuff in like fast forward toggle.

As for other gestures… for these types of controls will conflict with the upcoming free placing stick a/o d-pad and other revised touch controls i've got planned for 2.0. Apart from that, gestures need to be intuitive to what they do, and if not would require some onscreen tutorial/edu UI to establish it or if not that some visual feedback when using gestures (like if you were pinching having it shrink the screen as a snapshot confirmed saved and have it show that it saved that moment with something like a confirmation at the end but still…think a better UI to house these quick controls would be the way to go.

Another option would be to revise how the pause button works—I'm thinking something that slides out from the main button and you can slide to quickly, and release which will call that action instead of pausing… real quick action, but out of the way.

Layout concerns…

As for layout issues, I'll take a look when I've got a chance.

Save over last manual save behavior…

Gotta agree with Joe on this, quicksave should have it's own type, and only write to it, even if it's a and extra backup save slot or whatever, similar Autosaves. Taking a page from modern games. You don't want to be overwriting manual saves.

For a cleaner UI been thinking to change this screen to a vertical scrolling list, with auto and quicks at the top, and manuals under, with small/thin seperator and visual way to codify them or mark them, vs the grid used now, to avoid gaps in design layout due to orientation and devices, taking som inspiration from Skyrim

[Screen] AUTO - Auto Save 3
[Screen] AUTO - Auto Save 2
[Screen] AUTO - Auto Save 1
——————————————
[Screen] QUICK - Quicksave 2
[Screen] QUICK - Quicksave 1
——————————————
[Screen] 053 - Manual Save
[Screen] 012 - Manual Save
[Screen] 001 - Manual Save
…and so on

@sevdestruct
Copy link
Collaborator

sevdestruct commented Feb 3, 2019

Gestures comment… revisited

I misread Joe's comment earlier. We could, as he mentioned use an edge swipe — like make a narrow quick action bar UI that slides out via edge with button icons. I've updated my reply above.

@mymikemiller
Copy link
Author

mymikemiller commented Feb 6, 2019

Thank you for your feedback, guys!

Updating PVSaveState

I started work on separating quicksaves into their own section of the save state picker and ran into all kinds of problems trying to get PVSaveState to store the type of save using an enum [auto, quick, manual] instead of the boolean isAutoSave it uses now. I kept crashing during the migration because the new saveType enum wasn't showing up in newObject in migrationBlock of realmConfig. I guess Realm just doesn't work well with enums. I'm sure I was messing up something simple.

In any case, I reverted that attempt and switched to simply adding another boolean: isQuick. This has the benefit of being less likely to mess up people's saves due to migration as it can just be given a default value of false. I'm not so fond of this as it introduces undefined behavior in the case of isAuto == true and isQuick == true. It would, however, make future saves compatible with previous builds as we wouldn't be removing isAuto from PVSaveState (though I guess we don't necessarily have to remove it in the enum case... If we left it in, I think quicksaves from the future would show up as manual saves as they set isAuto to false). Not sure how important it is to be able to open save states in a version older than the one it was created in. I'm curious to hear your thoughts on this.

Save state picker design

Personally, I would want the quicksaves listed before the autosaves in the save state picker. I quicksave intentionally at memorable moments (A lot! E.g., when I'm starting a battle in Final Fantasy, I always quicksave. And again when I successfully finish the battle). Most often when I go to load a save, I'm trying to go back to the most recent quicksave I intentionally made. This is the behavior I expect out of a quickload button (or gesture), so I'd expect to see those predictable quicksaves first in the list. The autosaves are a nice backup in case I go too long without saving, but honestly that's rare for me.

If there's a quickload, I won't need to pull up the save state picker very often so I guess that screen doesn't matter that much for my style of gameplay anyway. In fact, when I actually do go to the save state picker, it's probably because I want to go back to something other than the quicksave that I would have just used the quickload to load. Now I'm rethinking everything I said in the previous paragraph...

Maybe it makes the most sense to have them in reverse chronological order, with types all mixed together (as long as they show what type they are, as in your design). That way you can quickly scan through the thumbnails from top to bottom and stop at whichever one is far back enough, regardless of how it was created.

@mymikemiller
Copy link
Author

Button

Because of the frequency with which I tend to quicksave, I much prefer an always-available button. I press quicksave more often than I press start or select in most games. I'd rather see the start and select buttons hidden behind a quickbar and have the quicksave button easier to access (/s. I don't really want start and select to move, just illustrating a point). I often find myself reacting to something happening in the game by quicksaving as fast as possible (e.g. when an enemy is coming toward me). If the enemy touches me and I die, I quickload and try again. If I wasn't able to quicksave fast enough and the enemy was able to make it a few steps closer to me when the save occurred, I may end up with a worthless save state that instantly kills me when I load it. I then have to go back to a previous save state (which is where a quicksave history of a few states in the save state picker will come in handy).

For my personal build, I plan to add quickload by long-pressing the quicksave button. Since I have "Show Quicksave Button" as an option under Saves in the menu (though maybe it belongs under Controller?) and it defaults to off, can I leave that in the PR (in addition to adding Quicksave to the pause menu)? If you prefer, I can add the button in a separate PR once the quicksave feature is in.

Here are some screenshots of the button as it is now. It stays nicely out of the way in both landscape and portrait and is easy to access with the right thumb (It's important that I can perform a quicksave/quickload while keeping my left thumb on the d-pad so my character instantly moves in the desired direction once the state is loaded).

@JoeMatt
Copy link
Member

JoeMatt commented Feb 6, 2019

Realm does work with enums but they have to be backed by a realm compatible associated value and you have to do the conversion yourself.

https://medium.com/it-works-locally/persisting-swift-enumerations-with-realm-io-dab37cd98bcd

Also you can't just remove an iVar from a Realm object without adding a migration function to the init of the database. You'd have to take all the old objects that have isAutosave and convert their values to the new enum associated value containing iVar on the newObject in the migration method.

You're not updating the save state's themselves so you don't need to worry about backwards compatibility, it's just the reference to those save states and extra metadata that's in the database, the saves themselves are raw files that are created and read by the cores which don't know anything about the Realm references.

@JoeMatt
Copy link
Member

JoeMatt commented Feb 6, 2019

I think the reasoning to need a quick save button on screen because you want to constantly tap it incase you die at any instance, is really a case for adding rewind functionality. Franticlly using quick save in rapid succession seems like a hack that fills the niche that rewind serves.

In either case, I'm not sure still that an always visible icon is the way to go, considering no one has ever made a feature request for quick saves before, I'd wager it's not a particularly high demand feature to warrant permanent real-estate. I suppose as long as it's an option that's off by default it would be ok.

@mymikemiller
Copy link
Author

Points well taken. None of the emulators I grew up with (mainly snes9x) had a rewind feature, so I guess I just got used to spamming quicksave. It still does come in handy for cases when you know you might want to return to a specific point, like when you have a choice of two directions to go and you know the one you choose might lead to a dead end or nothing of interest. Saves the time of walking all the way back, and rewinding through everything you did on the wrong path would be tedious.

I'm looking forward to the rewind feature - you may just get me to give up save states entirely :)

Thanks for the link re: Realm and enums. I'll see if I can get my attempt to compile.

@mymikemiller
Copy link
Author

mymikemiller commented Feb 18, 2019

The recent changes address your suggestions. "Quicksave" is now available in the menu, and the quicksave button can be toggled on in the settings (it is off by default). Quicksaves are separated from auto and manual saves in the Save States screen. Like auto saves, your 5 most recent quicksaves are kept. Long-pressing the quicksave button performs a quickload (loading the most recent quicksave).

I made a change to the way autosaves (and now quicksaves) are auto-deleted when there are more than 5. Previously, realm.delete(state) was used. This removed the state from the list, but did not remove the state files (.svs, .jpg & .svs.json), which just kept piling up. Now PVSaveState.delete(state) is used, which properly deletes these files. Not sure what other ramifications this change might have, so I wanted to call it out.

Another change ensures that the .svs.json file is deleted when a PVSaveState is deleted. Previously these files were left around indefinitely.

A couple things I'd like to call out:

  1. I didn't spend time making the save icon perfect. In its pressed state, I tried to make it match the pause icon's pressed state (i.e. it should have a white rectangle around it). I added the rectangle, but it doesn't match the pause icon's rectangle exactly. I figure you have the template rectangle and can easily add it to the save icon so everything matches perfectly.
  2. Having "Quicksave" in the pause menu causes "Swap Disc" to be pushed off the screen in landscape mode. The menu can't be scrolled, so there is no way to press "Swap Disc" when in landscape mode (I'm on an iPhone 10).
  3. In the Save States screen, the header ("Auto Save", "Quick Save" and "Save States") for sections without any quicksaves should not appear. If the user has never created a quicksave, they shouldn't see the "Quick Saves" header in the Save State picker. Same for auto saves. I couldn't figure out how to hide headers programmatically. I think it has something to do with referenceSizeForHeaderInSection, which I think would require a custom UICollectionViewDelegateFlowLayout, but I wasn't able to get that working. I'm happy to spend more time getting this to work, but I figure you plan to overhaul that menu anyway, so I just left the headers in for now.
  4. I'd like to use 3d-touch on the quicksave icon to cause a quickload (instead of a long-press which this PR implements). I figure it's harder to accidentally do a 3d-touch than it is to accidentally do a long-press. I'd also like some taptic feedback when the quicksave button is pressed. Currently all taptic feedback is handled by the PVControllerViewController, but this isn't a controller button so some refactoring would be required there.

Please let me know your thoughts on this PR. I've been enjoying using the quicksave feature, and I hope other users will find it handy too.

@QuarterSwede
Copy link

Just my non-dev/user two cents: I use an MFi controller which eliminates the on screen button overlay. I would like gestures (as well as the on-screen save button) or a button combo to access functionality while using a controller. I do love the basic idea of quick manual saves/loading.

Using realm.delete as was previously done does not delete the save state files
This re prevents the emulator from freezing when loading a quicksave by long-pressing the quicksave button when anothercontroller button is held down
Quicksave now only appears for cores that support save states. Also resolves a bug where using Quicksave from the pause menu caused video to freeze.
@mymikemiller
Copy link
Author

mymikemiller commented Mar 10, 2019

This commit made a lot of formatting changes that made merging this PR non-trivial. It has been rebased on top of those changes to be easier to diff and merge :)

I hope you'll accept this implementation of quicksave, and if not, let me know what other changes you'd like to see made.

@JoeMatt
Copy link
Member

JoeMatt commented Mar 12, 2019

I got a lot going on at the moment, I'll try to get to this soon but it may take a week or so. I want to test this and fix a crash and put a new build out so I need to find a few hours. Just been swamped lately, apologies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires feedback pending unanswered question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants