trishume / QuestSaberPatch

Patcher tool to add custom levels to the Oculus Quest version of Beat Saber
MIT License
64 stars 8 forks source link

Remove songs, FileSharing fixed #2

Closed sc2ad closed 5 years ago

sc2ad commented 5 years ago

Okay, so, I finished up most of what I wanted to do, I made some way of removing. However, I want to mention a few things first:

  1. Currently, the removal process DOES NOT use a Transaction for Assets. It does, however, use one for the APK. This doesn't seem too hard to fix, I just didn't do it yet. If you want me to, I can, but not until tomorrow.
  2. When objects are removed, they are removed and their PathIDs are reassigned after each LEVEL is removed (this is to avoid problems with pointers). Currently, the code functions fine, however, if some other thing were to have a pointer pointed to one of the objects that are destroyed using this script, then the game would not boot unless this pointer was forcefully changed. Again, this doesn't seem too hard to fix, involves some iteration and overall slowness and it may be impossible to be 100% confident that you renamed all pointers without completely translating the .assets file into legible code, but it would probably be good enough. Again, as it stands right now, The PathIDs get adjusted. However, This doesn't have to happen (as far as I can tell) so you could probably get away with removing the one line that calls EndRemoval in the JSONTypes class.
  3. I have tested it locally (not on a quest) and it seems to work fine. The way I made removing function within the app project was that by adding the -r or removeSongs in your command line args would remove any of the songs that are BOTH in your folder of local songs (the folder originally used to copy songs to the Quest) AND songs that exist on the quest. If a song exists in the local folder, but not on the quest, and you are running with -r, the song will still be ADDED. I did this mostly for testing, so if you want to change it, go for it.
trishume commented 5 years ago

I haven't done a full review of this yet, will take a closer look tomorrow. But one thing I'll note is that removal shouldn't involve JSONTypes except with maybe a helper, you should be able to delete a level just based on its levelD field without having the original JSON version on hand, all the info needed to delete it should be in the assets. It looks like you already mostly use the asset data so this mostly will involve moving the method to SerializedAssets

Also I put your Windows fix in ahead of time since it's more urgent, which means this PR needs a rebase/merge.

trishume commented 5 years ago

An idea I had: by analogy to tracing garbage collectors, you can use the same routine to find the pointers you need to delete on the object you're deleting, and find the pointers you need to fix up on objects where you're fixing up the pointer pathIDs. Have a method on AssetData and BehaviorData like void Trace(Action<AssetPtr> del) that calls the delegate for all AssetPtrs in the asset.

sc2ad commented 5 years ago

Updates

I have made quite a few patches, the most important being the addition of removals, allowed for a command line argument for replacing the existing texture for the ExtrasCollection, added splitting of assets after assets was saved (fixed some issues when installing), reassigning of PathIDs via any object.Trace, and verified all of the following by signing and verifying the existing .apk.

Command line arguments are as follows:

Next Steps

  1. Add support for creating and deleting existing and new LevelCollections and LevelPacks
  2. Add handling for SpriteAssetData, for allowing LevelPack texture remapping
  3. Handle support for more types of MonoBehaviors and AssetData, including Sabers, Environments, etc.
  4. ???
trishume commented 5 years ago

@sc2ad Check out #5 which I'm going to merge shortly. It should fix the problem that lead you to implement re-splitting so you can probably get rid of the re-splitting. Previously the manifest wasn't updated when we added/removed files but now the manifest is re-built.

Although keep the code around in Git history because it might be good for performance, or it might be bad for performance I really don't know. Might be worth testing with a huge library and see if we can notice any frame rate or loading time differences. I can see it going either way, so for now I'm inclined to keep it simpler for patcher performance and reliability reasons.

sc2ad commented 5 years ago

This PR is now SIGNIFICANTLY outdated. For that reason, I am closing it.