-
-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add background blur #5720
base: master
Are you sure you want to change the base?
Add background blur #5720
Conversation
src/microbe_stage/MicrobeCamera.tscn
Outdated
mesh = SubResource("1") | ||
material = SubResource("ShaderMaterial_1sr76") | ||
|
||
[node name="SubViewport2" type="SubViewport" parent="."] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be implemented as a second render pass or directly baked into the background shader? I ask because we'll hit a rendering performance problem at some point if we just throw subviewports at any problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it seems possible to re-implement this without viewports. I've just found out that it's possible for a shader to get it's texture from a CanvasLayer instead of a viewport (the docs), which should be more performant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case that's not usable, I think putting the blur code directly into the background shader would be the right way to go. That would also quite neatly I think expose how expensive in terms of texture reads it is actually to perform a blur.
Edit: and as an added benefit the blur could be only applied to the layer that has the distortion on it (so only layer0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the blur code into the background shader would be bad for performance, because it would then need n*n texture reads (for blur diameter of n), and for each of those reads it would also need to calculate distortion.
The current implementation does blur in 2 passes (vertical and horizontal), and because of that only needs 2*n reads. This difference in calculation costs should probably compensate for viewports' performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still it will now blur the moving bubble layers that are separate from the furthest background layer as well, which is probably not the best. Also it could maybe be possible to use a cheaper blur method combined into the distortion math to do a reasonable enough blur with high performance at the same time as applying the distortion math?
I guess I'll let you investigate first what is actually possible to do before dumping more ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently the way of making multi-pass shaders with CanvasLayers requires reading the screen texture, so there still needs to be at least one viewport.
The background plane can also be optimized into a ColorRect (which needs a scale argument passed to it, but it's probably still more performant than rendering 3D geometry).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about the situation more: I guess the most optimal way to implement this feature is to use the subviewports, but I think that to balance that out we should add a graphics option to control the blur amount and if the blur is zero the subviewports should be skipped entirely so that the game is no slower than it currently is on computers where rendering to texture ends up harming performance a lot.
(the subviewport chain still aplies regardless of blur)
Also add a temporarily unimplemented blur amount option
I've made the background plane change it's parent in the node tree to either the SubViewort or the camera depending on if blur is enabled. Blur planes' viewport texture is also being removed when blur is off, as it seems to be the only way to disable subviewports rendering. I have also moved all microbe background-related logic to a separate class as keeping it in the camera class wouldn't be clean. |
Doesn't subviewport have the render mode property that can be set to none / never to disable rendering it? At least I recall doing something like that with the PhotoStudio where I can trigger the subviewport it uses to render just a single frame and then auto-disable. Well the camera class is called MicrobeCamera, but I suppose separating out the complex background handling is a good idea anyway. |
…l checks that were needed because of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the code there's just a few small readability and style enhancements I'd like to see. I playtested this now and noticed that it seems like the background light level applying is broken. I'm here in epipelagic and despite waiting for basically a full day the background brightness never reaches the usual value:
Besides that one more minor point is that if the blur only applies when distortion is enabled the slider should be disabled when distortion is off to clearly indicate to the player what it does (and the checkbox should be moved immediately below the slider). Alternatively I think the blur could be made to also work with the normal backgrounds (though in this case I think the render resolution problem is a more important one). Also depending on how expensive the blur is when rendering at bigger size there might need to be a separate toggle about using low resolution blur on the backgrounds.
So yeah sorry to dump so much work still on you but I think this feature still needs polish before adding to the game.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also one thing in general is that the subviewports default to 720p. So even when running the game at 4K the backgrounds are at 720p so they are extra blurred due to the resolution. I'm not sure if anything needs to be done about that but I think at least a big code comment should be added to explain this situation.
An example fix would be to just check the game window size in the _Process here and resize the viewport sizes to match the window size if changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it work so that the viewport change is applied only once two frames the window size is the same? Or maybe there's like a 0.3 second interval that the resolution must be the same before switching (can be easily made with just a single double variable keeping time)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still results in a flicker, though it's better than continuous artifacts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I suppose maybe hiding the stuff entirely for the frame it is resized would remove the artifacting? But if that's worth the time to develop that kind of complex workaround, I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the background viewport is the one that causes those arifacts to appear, as rescaling only the blur viewport works fine. Could the problem be with rescaling specifically a 3D viewport?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the normal part of the game display fine though when resizing? So the issue is likely more complex with the different viewports depending on each other, which is likely the interaction that shows the non-optimal behaviour. Though of course I can't say anything for certain without checking Godot open and closed issues to see if this is something seen by other people as well.
I had just that one minor comment about the code for now, so the code so far written now looks good to me. I didn't notice any of the viewport resizing etc. code or allowing blur even without distortion enabled, so I guess those aren't done yet? I think it would take a bit of crunching if this is to make it into 0.8.0 so it is fine if this needs more time in a relaxed manner to get done, which I'd also prefer as I'm still not fully done with the re-added mac support so it would be a pretty big squeeze to fit this into 0.8.0. |
Looks really nice. I think it might be worth it to increase background distortion speed now, as the movement of the background is even less obvious now. The blur should help alleviate worries of disorientation we had before, where the background might have moved too fast for the eye to be comfortable with. This isn't relevant to this pull request, but in the future, it would be nice to create some sense of depth, as it can sometimes seem obvious that the background is the background and the plane the cell is on is the foreground. |
I think that as the blur might be performance sensitive, we need either a toggle for the blur to be lower resolution. Or we should allow separately toggling the blur and distortion effects so that someone on a lower power device can still play with the distortion on even if the blur takes too much performance. |
Maybe it would be a good idea at this point to make a separate graphics options section for the microbe background? It would probably be more clear that way, and would allow not writing "background" before each of the new options' labels |
Yeah that probably makes sense. Translation keys should still be clear and include the context that they are about a game background for translators to be able to translate correctly. |
We are currently in feature freeze until the next release. |
@@ -853,6 +856,8 @@ public void ApplySettingsToControls(Settings settings) | |||
bloomEffectToggle.ButtonPressed = settings.BloomEnabled; | |||
bloomSlider.Value = settings.BloomStrength; | |||
bloomSlider.Editable = settings.BloomEnabled || !DisableInactiveSliders; | |||
blurSlider.Value = settings.MicrobeBackgroundBlurStrength; | |||
blurSlider.Editable = Settings.Instance.MicrobeDistortionStrength > 0 || !DisableInactiveSliders; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the effect look really bad without distortion, or why was this ultimately left so that the blur cannot be enabled unless distortion is also on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's honestly debatable if blur should only be possible to enable with distortion. My reasoning was that the backgrounds were originally not designed to be used with blur, so they might look bad with it.
But if blur is to be applied to them without distortion, it's probably better to just switch the images to pre-blurred ones for more performance. The cost of that would be blur strength being unconfigurable, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue the distortion already changes the original artistic vision, and I allowed that so with the same logic blur should be allowed. And as it would be just a tiny bit of code change to not lock out that option while distortion is off, I think it is worth it to allow it. And blurring the background images themselves would need a lot of artistic touch to do it well (it was tried once before and I really disliked the effect), and no one would be able to play with the old backgrounds if they preferred. So all in all I think it is just much superior option to allow to enable the blur separately from the distortion effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the pre-blurred images would only be shown when blur is on (otherwise the original backgrounds would remain); though it might feel weird that the slider doesn't control blur strength when distortion is off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't want to bloat the game filesize a bunch just to have this feature. So I'm against including both variants of the backgrounds, which is why I think this should be a dynamically rendered effect.
Brief Description of What This PR Does
Adds a Gaussian blur to microbe backgrounds (only enabled if distortion is on).
Related Issues
Closes #5641
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.