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

Make all post-processing conditional #2173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Denneisk
Copy link
Contributor

Optimizes post-processing by not having their hooks run when they're never used. Also removes some unnecessary checks or moves them to run only once.

Now post-processing hooks run only when needed.

I tested it and it all works fine, but I would appreciate anyone looking over this to make sure I didn't do any copying errors.

Was waiting for someone else to do this before I realized it'd be faster to do it myself.

@Cheatoid
Copy link
Contributor

Cheatoid commented Jan 1, 2025

I like the idea, but:

  • Change this logic: newValue != "0" to check particular convar's boolean value instead (ConVar:GetBool()), this is because any non-zero value is considered true in engine, even -1 and 123.45. Consistency.
  • Have you considered what would happen if any of those Lua files were (hot)reloaded (or loaded twice for whatever reason), no? Well, it would add another instance of those change callbacks into the list, and there be double shenanigans.
  • In regards to bloom/sunbeams/toytown, you can completely wrap those around the render.SupportsPixelShaders_2_0() check, because GPU isn't going to change while Garry's Mod is running, ay? (Basically no change callback even.)

@Denneisk
Copy link
Contributor Author

Denneisk commented Jan 2, 2025

Thanks for the response

Change this logic: newValue != "0"

I thought all nonzero values were true for convars. I thought it would be simpler this way.

(hot)reloaded

Thank you for considering this, that's a very important oversight.

render.SupportsPixelShaders_2_0()

Good idea. Thanks.

Comment on lines -62 to +80
The function to draw the bloom (called from the hook)
-----------------------------------------------------------]]
hook.Add( "RenderScreenspaceEffects", "RenderBloom", function()
cvars.AddChangeCallback( "pp_bloom", function( _, _, newValue )

-- No bloom for crappy gpus

if ( !render.SupportsPixelShaders_2_0() ) then return end
if ( !pp_bloom:GetBool() ) then return end
if ( !GAMEMODE:PostProcessPermitted( "bloom" ) ) then return end

DrawBloom( pp_bloom_darken:GetFloat(), pp_bloom_multiply:GetFloat(),
if ( newValue != "0" ) then
hook.Add( "RenderScreenspaceEffects", "RenderBloom", function()

DrawBloom( pp_bloom_darken:GetFloat(), pp_bloom_multiply:GetFloat(),
pp_bloom_sizex:GetFloat(), pp_bloom_sizey:GetFloat(),
pp_bloom_passes:GetFloat(), pp_bloom_color:GetFloat(),
pp_bloom_color_r:GetFloat() / 255, pp_bloom_color_g:GetFloat() / 255, pp_bloom_color_b:GetFloat() / 255 )

end)
else
hook.Remove( "RenderScreenspaceEffects", "RenderBloom" )
end

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it this way introduces a regression where the result of GAMEMODE:PostProcessPermitted is not updated live.

I can understand SupportsPixelShaders_2_0 being a static thing and can be moved outside of the hook.

I also think it's a better idea to make a the hook a local function and reference that in the cvars.AddChangeCallback callback, rather than nesting functions like this.

@@ -113,4 +117,4 @@ list.Set( "PostProcess", "#bloom_pp", {

end

} )
} )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change.


DOF_SPACING = pp_dof_spacing:GetFloat()
DOF_OFFSET = pp_dof_initlength:GetFloat()
end)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change that does not conform to the style guidelines:
https://github.com/Facepunch/garrysmod/blob/master/CONTRIBUTING.md


cvars.AddChangeCallback( "pp_fb", function( _, _, newValue )

if ( !GAMEMODE:PostProcessPermitted( "fb" ) ) then return end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "frame_blend", not "fb"


if ( !frame_blend.IsActive() ) then return end
if ( !frame_blend.IsActive() ) then hook.Remove( "RenderFrameBlend", "RenderFrameBlend" ) return end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary?


vgui.GetWorldPanel():MouseCapture( false )
FocusGrabber = false
end )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file is a mess. See previous comments.

I also think this can be all done better, such as removing the hooks from Panel.OnRemove of the SuperDOFWindow for example.

@robotboy655 robotboy655 added the Enhancement The pull request enhances current functionality. label Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement The pull request enhances current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants