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

Combined the DOF far and DOF near passes #50723

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

BastiaanOlij
Copy link
Contributor

While working on adding a fragment based DOF implementation for the mobile Vulkan renderer in Godot 4, I did this optimisation that made a lot of sense to retrofit into Godot 3.

Currently Godot will apply far DOF in post processing first, then apply near DOF next each costing at least 2 passes, with GLES3 adding a merge pass. That is 5 full screen passes to apply DOF.

When we look at DOF settings:
image
We see that we have a DOF setup that:

  • if depth is smaller then 2.0 is fully out of focus (near)
  • between 2.0 and 2.5 it goes into focus (near)
  • we're in focus from 2.5 until 10.0
  • when we slowly go out from 10.0 to 13.0 (far)
  • and we're fully out of focus for any depth greater then 13 (far)

We can combine the blur amount calculation in the shader with very little difference in effect to doing separate passes. There might be some slight variation when there is a crossover between an object out of focus in the foreground while the background is out of focus as well but the effect is overall the same.

As a bonus I've also added an extra check that makes sure it no longer does a bunch of needless look ups for a fragment that is in focus.

Stable 3.3.2:
image

New version:
image

The only small "breaking" change is that when both far and near DOF are turned on you can only use one amount and one quality settings. dof_blur_near_amount and dof_blur_near_quality will be ignored when both are on. I decided against merging the settings because that would break peoples games fully especially if they are only using near dof.

It would be great if people could test more elaborate scenes and do some before/after comparisons to make sure this doesn't present any unexpected side effects and if someone could do some performance tests.

@clayjohn
Copy link
Member

Do you notice a performance difference on your device?

@BastiaanOlij
Copy link
Contributor Author

@clayjohn My laptop has a 2070 in it so with a scene as simple as this it probably won't be noticable

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 22, 2021

@clayjohn ok, running this project full screen on my laptop with v-sync off:

  • in 3.3.2: 245 fps
  • with this PR: 282 fps
    (Laptop with a GeForce RTX 2070)

I call that an improvement :)

@akien-mga
Copy link
Member

The only small "breaking" change is that when both far and near DOF are turned on you can only use one amount and one quality settings. dof_blur_near_amount and dof_blur_near_quality will be ignored when both are on. I decided against merging the settings because that would break peoples games fully especially if they are only using near dof.

I think the small compat breakage is OK for 3.4, but I'm a bit concerned that the UX is a bit confusing. Without reading the docs, users wouldn't know why their near configuration gets overridden.

It's maybe not too bad as it's properly called out in the docs, though.

The alternative would be to reorder and rename properties, and handle the old names in _get and _set for compatibility, as a proxy to the main amount and quality properties (and throw a warning if both the near and far properties are set). But I don't know if that's worth the effort.

@BastiaanOlij
Copy link
Contributor Author

@akien-mga I agree but honestly, who changes those settings between the two anyway? What can you possibly gain from running foreground in low and background in high or visa versa.

@Calinou
Copy link
Member

Calinou commented Jul 22, 2021

I think the small compatibility breakage is worth it too, as long as it's mentioned in the release notes and changelog.

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Jul 22, 2021

You could merge Quality and amount if you just make DOF one setting instead of 2. Here's what I mean:

Before:
DOF near
-enabled
-distance
-transition
-amount
-quality
DOF far
-enabled
-distance
-transition
-amount
-quality

After:
DOF
-near (on/off)
-near distance
-near transition
-far (on/off)
-far distance
-far transition
-amount
-quality

4.0 already has this sort of setup (quality is just a set of Project settings tho) and it could be for the most part just a UI change

@akien-mga
Copy link
Member

akien-mga commented Jul 22, 2021

@mrjustaguy Yes that's what I have in mind, but this breaks compatibility, so it needs extra compatibility code to properly convert the properties in 3.3 projects to the new properties. Otherwise users upgrading to Godot 3.4 will lose their DOF configuration.

That requires a significant amount of code wrangling in Environment and VisualServer.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I'll merge this for now.

We can keep discussing if the settings should be harmonized and compatibility code should be added to handle the 3.3 -> 3.4 compat breakage, but since that would be a superset of this PR it can be implemented later on.

@akien-mga akien-mga merged commit 214106d into godotengine:3.x Jul 22, 2021
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij
Copy link
Contributor Author

I'll merge this for now.

We can keep discussing if the settings should be harmonized and compatibility code should be added to handle the 3.3 -> 3.4 compat breakage, but since that would be a superset of this PR it can be implemented later on.

Remember that this means you also can't go back...

@mrjustaguy
Copy link
Contributor

I'm unsure of exactly how the codebase is for this, but I can see a possible way to not break compatibility and do the UI change..
The data is all the same, so really only the editor has to interpret it differently (as in have the effects be in DOF instead of DOF Near/Far and a UI rename to settings), as there were no additions to the data, and no actual removals (near amount and quality are just ignored, not removed)

Of course if this part is more tied to the UI it could be a problem, which would probably require some breaking, or a hacky solution.

@BastiaanOlij BastiaanOlij deleted the combine_far_near_dof branch July 22, 2021 13:12
@BastiaanOlij
Copy link
Contributor Author

I'm unsure of exactly how the codebase is for this, but I can see a possible way to not break compatibility and do the UI change..
The data is all the same, so really only the editor has to interpret it differently (as in have the effects be in DOF instead of DOF Near/Far and a UI rename to settings), as there were no additions to the data, and no actual removals (near amount and quality are just ignored, not removed)

Of course if this part is more tied to the UI it could be a problem, which would probably require some breaking, or a hacky solution.

You are going from 2 sets of settings to 1 set of settings, that means giving one set up. If we rename the far set and remove the near set, then someone who is only using NEAR DOF will see their game break.

With this approach someone who is only using far DOF is happy
someone only using near DOF is happy
someone using both may see a change if they were using different near settings to far which is unlikely.

In Godot 4 we do have this breaking change and have only one set of quality settings that apply to both near and far DOF.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 26, 2021

I'll merge this for now.

We can keep discussing if the settings should be harmonized and compatibility code should be added to handle the 3.3 -> 3.4 compat breakage, but since that would be a superset of this PR it can be implemented later on.

Cool!

Just a last word from me on merging the settings, they are merged in Godot 4 and I think that is enough. I switch a project between the current stable 3.3 branch and my 3.x development branch numerous times and breakage like this will cause issues in settings 3.3 relies on disappearing from the project. While I don't expect DOF to cause me much trouble I can imagine others might be less happy about it when they loose settings as a result. They may not care about not being able to set a separate near and far DOF value as they are often the same, but they will care if the settings keep disappearing on them because they have been renamed between versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants