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

Respect BGBrightness when StepStatistics are on #166

Merged
merged 1 commit into from
Oct 29, 2019
Merged

Respect BGBrightness when StepStatistics are on #166

merged 1 commit into from
Oct 29, 2019

Conversation

natano
Copy link
Member

@natano natano commented Oct 28, 2019

Currently the background is displayed at full brightness when the step statistics are on, which can be quite distracting.

It took me quite a while to find the right diffuse value (couple of hours of reading SM source and trying stuff out), but it seems to visually match with what the StepMania BG implementation does. I compared some screenshots with step statistics on and off at different brightness values.

I usually play with HideSongBG, but I noticed that this is wrong, so I wanted to fix it. It wasn't trivial for me, so I thought it might be worth sharing it. ;)

Currently the background is displayed at full brightness when the step
statistics are on, which can be quite distracting.

It took me quite a while to find the right diffuse value (couple of
hours of reading SM source and trying stuff out), but it seems to
visually match with what the StepMania BG implementation does. I
compared some screenshots with step statistics on and off at different
brightness values.

I usually play with HideSongBG, but I noticed that this is wrong, so I
wanted to fix it. It wasn't trivial for me, so I thought it might be
worth sharing it. ;)
@quietly-turning
Copy link
Contributor

Thank you for taking the time to find this bug, spend the necessary hours and hours digging into it, fixing it, testing it, and submitting a contained pull request with a helpful and clear description. Your efforts are very much appreciated.

I don't have a working copy of SM5 to test this on, but your changes look good to me. I'll merge this now.

@quietly-turning
Copy link
Contributor

When I introduced the horizontally scrolling graph, my original implementation just used a black Quad all the time (to hide to the scrolling ActorMultiVertex).

At that time, many people pointed this out as a bug. They wanted to see a little bit of the background peeking out around the edges of their screenfilter. This made sense to me, so I tried to accommodate in 5f05e10.

I had forgotten about the existence of the BGBrightness preference, so your fix here is appreciated.

Other players elsewhere are pointing out that this system still doesn't support background videos: https://twitter.com/symbiote_k/status/1178160405459263490

Recreating the engine's gameplay background code to that point seems like overkill to me. It would make more sense to crop the graph as it scrolls and not need to ever hide any part of it as it scrolls.

The histogram is currently drawn using an ActorMultiVertex and those do not currently support all base Actor commands, notably cropleft() and cropright() which would be useful here.

I've considered rendering the graph to texture using an ActorFrameTexture, because the Sprite it would then appear in would support basic crop commands, but then a major theme feature would require RTT and the theme would no longer support D3D.

Do you have any thoughts on this as a developer and (presumably) a person who plays StepMania?

@quietly-turning quietly-turning merged commit e7be131 into Simply-Love:beta Oct 29, 2019
@natano
Copy link
Member Author

natano commented Oct 30, 2019

You are right, it would be better if the density graph wouldn't clobber the background in the first place.

I think I'll have to play around a bit more with different approaches to find out what' might work instead. At the moment I see two options:

  • RTT with either not supporting D3D at all or providing the current hack in case RTT is unavailable (meaning that D3D wouldn't support video BGs, but other backends would).
  • Remove vertices from the density graph that would be outside of the range. I don't know what performance/framerate impact this would have though. This would allow to support BG videos everywhere. I will investigate in the feasibility of that approach.

@natano natano deleted the stepstatistics-bgbrightness branch October 30, 2019 21:48
@quietly-turning
Copy link
Contributor

quietly-turning commented Oct 31, 2019

Remove vertices from the density graph that would be outside of the range.

I've definitely considered trying this.

I have not dug too much into the relevant source code here, but my understanding is that when themeside Lua changes the table of vertices and passes that table back to the engine via SetVertices() so it can be (re)evaluated, it's that initial reevaluation that hurts performance.

I didn't appreciate this with my earlier implementations of the NPS histogram and LifeMeter line that come with SL's StepStatistics, so I had the ActorMultiVertex LifeMeter updating and adding more vertices every 0.333 seconds.

This started off fine enough, but with very long songs it meant that the LifeMeter line's performance would continue to get worse and worse as the engine's internal data structure grew larger and larger and needed to be entirely reevaluated every 0.333 seconds.

This eventually lead to pull request #93 from @andrewipark which put a limit on how many vertices the LifeMeter could have and scaled the sample rate to accommodate the duration of gameplay.

I'm mentioning all this here because I like the idea of pruning vertices from both the Histogram and the LifeMeter line as they scroll out of view. I'm thinking reasonable vertex limits could be chosen for both AMVs and then kept constant during gameplay. As the graphs scroll, some vertices are removed and others are added.

The idea seems clean, feasible, and it might just improve performance in gameplay and fix this silly need to hide the scrolling AMVs behind partial pieces of background...

@quietly-turning quietly-turning added the enhancement Improve existing functionality label Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants