Refactor density graph rendering (BG video supported!) #167
Conversation
|
Woah. I admit I wasn't expecting this, and am immensely grateful for and humbled by your enthusiasm and tangible contributions. Thank you! This looks like it was a lot of work. How long did you spend working on this?
Yeah, my code there was ugly for sure, so don't worry about it. I rushed that out in 2018 as part of an an effort to completely abandon the theme after the v4.7 release. I should probably go back and clean it up someday... I don't have a working build of SM5 right now, so I'll need to rely on input from other people who can test this and report on FPS during gameplay. Thank you for working on this! |
|
It looks like this effectively undoes the fixes from #153. Was this intentional on your part? |
|
Thank you for your kind words :) I would guess I worked on this for about 10-12 hours. I like this theme a lot, so it's worth the effort!
No, that was just me pulling everything apart and not assembling it back correctly. I assumed the first second is always >= 0. If the first second is < 0 it makes me wonder where the life bar should start... I will update the PR with a fix. |
|
I amended the commit to contain a fix for the issue described in #153. To understand the code it's necessary to understand how timing works in SM. This is what I found out:
Instead of always beginning the NPS graph at 0 the code now uses either 0 or the point in time where the first measure starts, whichever occurrs first. With that approach we truncate neither the lifeline nor the NPS graph. |
|
I also snuck another improvement in there to reduce vertex updates in the scrolling NPS histogram implementation. Now we only actually call |
|
@dguzek I did some performance tests today. The tests were down on my two laptops. I'm sure there are people out there running StepMania on slower machines, but at least on the two machines I have the new step statistics don't cause any slowdown. Both machines run Linux, with SM ofc using OpenGL. The Song used for testing is "Endless Sacrifice (14)" from the Ultramarathon Gigapack with 2x rate and autoplay. The song is > 10 minutes, so the graph scrolled during the test. I did this to make sure that scrolling also doesn't cause framedrop. Thinkpad T470 with Intel HD Graphics 620
Acer TravelMate B117 with some integrated Intel GPU
With the "old" density graph rendering (beta branch) the VPF is consistently higher, about 1700-1800. The longer the song the bigger the discrepancy. |
|
Thanks for putting in the time to test this, @natano! I still do not have a working SM5 build and won't for the foreseeable future, so this helps. Since you didn't specify, how did you collect your data for FPS and VPF? I'm guessing you enabled Did FPS never go above 60 in your tests? I'd found that with macOS builds of SM5, the This made it difficult for me to evaluate the performance of projects like Your Drifting Mind. My MacBook would report a steady So, I am wondering how you got the numbers you reported here. |
|
Yes, I observed the in-game stats. Your comment made me re-run the tests with vsync turned off. (I had to use this hack to really disable it on Linux with Intel HD Graphics.) Unfortunately vsync off does uncover an issue. The VPF count increases over the duration of the song and the FPS dips. I don't know what is going on there. Somehow vertices are leaking when vsync is off. I will investigate what is going on there. Thinkpad T470 with Intel HD Graphics 620
Acer TravelMate B117 with some integrated Intel GPU
|
Previously for a scrolling NPS graph all the vertices were rendered from the beginning and the screen edges and a "fake" background image were used to hide the overhang. After this only the vertices that should be visible are rendered. No overlap truncation required and all background features are suppored now: hiding, dimming, change, videos. The calculations for positioning the NPS graph with Center1Player is a bit messy, but I couldn't find a cleaner way to properly align it.
|
So, I found the issue: The high VPF count was caused by the life line!. When refactoring the code I removed the condensing of the life line. I deemed it unnecessary because we don't keep the whole thing around anymore but truncate it on the left. Turns out the line strip rendering in StepMania is quite inefficient. For line strip rendering every vertex in the ActorMulitVertex translates to actually about 38 vertices being rendered. With the life line condensing code back in place, the numbers look much better. This time I added debug output with the FPS/VPS numbers to the Update function and wrote a script to interpret them. Thinkpad T470 with Intel HD Graphics 620
Acer TravelMate B117 with some integrated Intel GPU
@dguzek I don't know what your perfomance goals are for the theme, but the numbers look acceptable to me. |
|
First off, thank you so very, very much for your incredible effort on this. It is uncommon to receive a pull request that:
This is one of the few pull requests I've received that has made things easier for me rather than harder. I've done no work here but feel fairly confident that I'm ready to merge this. I could go on all day, so I'll conclude by affirming that your programming efforts and words here are very truly appreciated. Thank you. 💛 So, yeah, I'm okay with merging this as-is; I just have one last thing I'll ask you to check. Your discovery that
makes me wonder how toggling I think there might be a performance improvement with |
|
The numbers abover where collected with SmoothLines off. Smooth lines do affect rendering efficiency, but not in the way that you would expect: Turning smooth lines on actually causes the OpenGL backend to use GL_LINE_STRIP and GL_DOTS (drawing about 2 real vertices for each vertex in the ActorMultiVertex) instead of the generic/non-smooth fallback code which blows up the vertex count 40 fold. So drawing smooth lines is actually more efficient than non-smooth lines with OpenGL! (D3D always draws non-smooth lines, even when the option is on.) Following number are with SmoothLines on. Note that the VPF number are too optimistic because the OpenGL backend seems (as far as I can see) to not record vertices used for smooth lines in the statistics. The FPS numbers should be fine. Thinkpad T470 with Intel HD Graphics 620
Acer TravelMate B117 with some integrated Intel GPU
|
|
This is interesting stuff. The FPS numbers look close enough that I don't think I'll change any of the help text. Thanks for looking into it on my behalf. Someday I'll achieve C++ proficiency and have a better understanding of the StepMania engine and then I will die of old age. In the meantime, the post-ITG community is surely in your debt. |
|
Here is a short video where you can see the refactored density graph in action. I'm a developer myself, so I know how it feels to get vague PRs. Glad I was able to provide you with a better experience! :) |
re: github.com/quietly-turning/Simply-Love-SM5/pull/167#issuecomment-549084002 "Yeah, my code there was ugly for sure, so don't worry about it. I rushed that out in 2018 as part of an effort to completely abandon the theme after the v4.7 release. I should probably go back and clean it up someday..."
So, I sat down today and implemented what we discussed in #166.
Previously for a scrolling NPS graph all the vertices were rendered from the beginning and the screen edges and a "fake" background image were used to hide the overhang.
After this only the vertices that should be visible are rendered. No overlap truncation required and all background features are suppored now: hiding, dimming, change, videos.
The calculations for positioning the NPS graph with Center1Player is a bit messy, but I couldn't find a cleaner way to properly align it.