Snow1226 / CameraPlus

Plugin for Beat Saber for a smoother and wider FOV camera
MIT License
122 stars 21 forks source link

Lagspikes when used in conjunction with Gotta go fast #7

Closed kinsi55 closed 3 years ago

kinsi55 commented 3 years ago

Hey,

I've created the afformentioned mod gotta go fast and seemingly at random some people reported to me that they get 2 lag spikes in the beginning of songs (Which can make some songs hard to play which start in the time period where the lag spikes happen), but only when they restart a song, if they just normally start a song they dont have it.

Since restarting a song does a scene transition to the menu and into the song again I assumed the issue has to be related to some mod that does some (probably unnecessary because they dont check if its a song restart) processing for the hidden menu transition. Having done some trial and error debugging, that mod seems to be Camera+ - 2/2 people who I've asked so far to test if removing camera plus removes the lag spikes said that it does.

I tried to do some basic debuggin with cam+ where I (If my code was correct that is) skipped the DelayedActiveSceneChanged implementation if the to scene isnt the most recent one that reached OnActiveSceneChanged but that did not seem to resolve the issue.

I understand that this might very well be something not in your interest to look into since the lag spikes normally fall into loading time, but maybe you have an idea what it could be caused by and would possibly like to resolve it anyways? :)

Thanks in advance

Snow1226 commented 3 years ago

It is probably the cullingmask process of cam+. This is the part where the culling process is done after the scene transition is stabilized. This is because the cullingmask of the game is different between the menu scene and the game scene since the game version 1.8.0. You need to switch the culling for cam+ that is displayed across both scenes. If the culling process is brought forward, a CRITICAL error will occur at irregular intervals, and this is already a specification because it is more likely to occur for people with lower PC specs.

kinsi55 commented 3 years ago

Would it be possible then to skip this process when you're restarting a song? In that case the cameras can just keep their culling mask and reuse it, right?

Snow1226 commented 3 years ago

So you're pausing and restarting GameCore? If so, that's different... It pauses and resumes the movescript, but I don't think it does anything else.

kinsi55 commented 3 years ago

No, my plugin does not modify the original flow of actions, but when restarting a song the game ofc goes back to the menu and then automatically starts the song again, hence me wondering if it would be possible(and if that would resolve the issue) to skip initialization of the menu cameras in that case and just keep reusing the previous gamecore ones so they don't need to be recreated

Snow1226 commented 3 years ago

I'm not a native English speaker, so it's a little difficult to understand. Would it be possible to rework the mod flow itself? I'll consider it, but I don't have any skills beyond maintenance, so I think it will be difficult.

kinsi55 commented 3 years ago

Ok I understand, I will go a bit more into detail:

When you restart a song what happens in the background is that the game will go back to the menu, like you pressed the menu instead of the restart button, but once it is in the menu it will automatically start the same song again - you never see the menu but it is loaded. This is obviously one of the reasons restarting a song is so slow by default.

By default this does of course not cause any issues because the song restart takes like 2-3 seconds so most mods (Including camera+) are done doing all of the stuff that they tried to process / load from going back to the menu, but with my mod the song restart time goes down to about half a second or less.

In the restart process Camera+ will of course follow the game flow and initialize the cameras for the main menu (Which is never seen) only to initialize them for the game scene again afterwards. What I was wondering is if it would help to know in advance that the scene switch away from the game scene is happening to restart the song, and in that case ignore it and make Camera+ keep the created cameras so that it does not need to do any heavy processing on the restart of a song.

Side note: Depending on what your native language is, if deepl is able to translate to it, maybe that helps you out in cases like this because its translation is significantly better than what Google is able to do.

Snow1226 commented 3 years ago

Restarting a song is when you stop the game you are playing and press Restart? Well, I'll think about it, but don't hold your breath. I also use DeepL. The conversion to Japanese is basically difficult.

kinsi55 commented 3 years ago

Restarting a song is when you stop the game you are playing and press Restart?

Correct, that is what I mean.

I will try to do some changes to Camera+ myself and see if anything works, if so I will let you know. Thank you for the consideration.

Snow1226 commented 3 years ago

I understand. Thank you very much.

kinsi55 commented 3 years ago

Alright I've done a lot of back and forth testing, this is what my very spaghetti code is right now. I would absolutely not consider it to be ready to ship but from my limited testing, at least basic camera+ usage still works completely fine, and most importantly, the lag spikes seem to be gone entirely.

https://github.com/Snow1226/CameraPlus/compare/master...kinsi55:master

Edit: If that is alright for you, I'm having a couple of people test and see if there is any major issues with these changes, if not I would release it as a modified version in BSMG for people to test and see if they find any issues so you get an idea if this breaks anything.

Snow1226 commented 3 years ago

Thanks for the sample code. I've tried it, but It looks like it's working, but the scene itself is reloaded and the CullingMask is initialized. So if I skip the culling process, it does not display as intended.😣

kinsi55 commented 3 years ago

Okay, I have another Idea, I will implement it a little bit later.

kinsi55 commented 3 years ago

I've implemented my idea now, seems like the culling mask was not even the issue. The way it is implemented now it sets the culling mask when going into the song again and still does not lag.

I had implemented it differently at first (commented code) where using some memory hacks I stubbed out the original mask setter, allowing to keep the previous mask, but that is unsafe code and apparently not really necessary so I removed it again.

I've added several cameras with different display options (UI / Debris / transparent walls) and they are all applied correctly, even after restarts.

If you try it and think its working fine I would release it on the discord and ask people to test it if you are okay with that.

Snow1226 commented 3 years ago

Thanks, I've tried it and Culling looks good, but the Noodle Extensions PlayerTrack tracking stops working. For example, this https://beatsaver.com/beatmap/11cf8 https://youtu.be/UHyAsWsP-VA I'll give this some thought.

kinsi55 commented 3 years ago

I see, well you get the general idea.

I'm assuming your latest commit is not complete but you should definitely consider implementing all of the changes I made to the DelayedActiveSceneChanged (Mainly the different method of waiting for Camera.main and shortened delays) as well as the processing bypass I implemented when restarting a song as they are also significant factors to decreasing the stutters.

Snow1226 commented 3 years ago

Except for the process bypass, I plan to adopt it because it works to shorten the process. Only the process bypass is troubling me. If I bypass it, the link to the origin point where NoodleExtensions is running will be broken. I'd like to reset it somehow, but due to the use of transform.parent I can't get it to work except for the camera reset.

kinsi55 commented 3 years ago

If the only issue with my implementation is that your Noodle implementation is broken I will try to find a solution for that, I'll let you know once / if I have one.

kinsi55 commented 3 years ago

I tried around a bit, I'm having a lot of issues even understanding how the control flow in camera plus works and everything seems very spaghetti code to be honest(Which is probably a result of how old this plugin is), but, the code in my repo now works with your noodle function.

I think Cam+ could benefit a lot from a full rewrite using "modern" tools like SiraUtil/Zenject and the like but that would obviously be a lot of effort.

Snow1226 commented 3 years ago

Thanks, I'll check it out after work. I agree about the rebuild, though my skills are lacking.

But there are a lot of restrictions because the update policy is not to rely on anything other than BSIPA. For example, even if CoreMod is late in updating, as it was in game 1.12.0, streamers can still make screens as long as they have a camera.

kinsi55 commented 3 years ago

I would rather make it a completely new plugin instead of an update if you were to rewrite it. If you wanted to make it an update, I think if you released it as a new major version (So V5.0) you should be able to depend on new things, but I am not exactly sure how Beatmods handles that.

I'm pretty new to BS / Modding it myself but I like challenges, I might look into possibly starting a rewrite and see how much effort it would take to get a basic implementation done (Only to realize how much effort it would actually be and cancel it again)

Snow1226 commented 3 years ago

I've tried. I was getting the following error as to why I had originally added WaitForSeconds. Again, when I shorten it, it occurs irregularly.

[ERROR @ 21:13:40 | CameraPlus] Exception cameras culling! Exception: Object reference not set to an instance of an object [ERROR @ 21:13:40 | CameraPlus] at CameraPlus.CameraPlusBehaviour.SetCullingMask () [0x000e6] in :0 [ERROR @ 21:13:40 | CameraPlus] at CameraPlus.CameraUtilities.SetAllCameraCulling () [0x00019] in :0

Also, on the development PC, the NoodleExtensions tracking seemed to be working but, on a stream PC, it sometimes didn't work even in the first play without restarting.😢

Snow1226 commented 3 years ago

I'll still check it out, but... Maybe I can set it to skip only when I have GottaGoFast.🤔

kinsi55 commented 3 years ago

While you probably could, tackling an issue like this with random number delays is very ugly hence me trying to look for a solution that does not need it

kinsi55 commented 3 years ago

Out of curiousity, in your system where it does not work, are you using the profile switcher? Can you maybe upload the config of the system where it doesnt work?

I've done some more testing, enabled the profile switcher and did some more changes to how the delays are implemented, the current solution looks like it should work, if you could test if it does for you that would be great.

Snow1226 commented 3 years ago

Oh, I see. I did use a profile switcher on my PC for the stream. And the code you've updated looks good. There was no Exception cameras culling during the retest. I'll test again tomorrow with streaming.

Snow1226 commented 3 years ago

I tested it by streaming for two hours. Everything was resolved, so I'd like to make a decision on this branch. Thank you for your cooperation.🎉