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

Add Nvidia Workaround for GLES3 #38517

Merged
merged 1 commit into from
May 8, 2020

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented May 6, 2020

Ported GLES2 workaround code to GLES3. This is to deal with the infamous flickering issue on some hardware / drivers.

Several people were asking for this in #9913.

  • As with GLES2, it seems to run less than half the speed of the uniform draw method.
  • If we add this, we need to decide what to do with the existing project setting gles2_use_nvidia_workaround. It would seem to make sense to lose the gles2 prefix and use the same for both. This may potentially confusingly make some people's project settings show both the old setting and the new, if they have existing projects that use it.
  • I've turned it off when running the editor, simply because it appears to give a slightly slower response in the editor to my eye, and we haven't had reports of flicker being a problem in the editor.

I'm planning to have a go at fixing the uniform draw method soon, if we can get that working it will make the workaround code redundant, but that's a big if! 😁

@lawnjelly lawnjelly requested a review from reduz as a code owner May 6, 2020 18:19
@Calinou Calinou added this to the 3.2 milestone May 6, 2020
@lawnjelly lawnjelly force-pushed the gles3_nvidia_workaround branch 2 times, most recently from b0c8f42 to 3aad68c Compare May 7, 2020 07:16
@lawnjelly lawnjelly requested a review from a team as a code owner May 7, 2020 07:16
@lawnjelly
Copy link
Member Author

Ok, code is now cleared. up. Only question now is what to use for project setting:

Project Setting Name

This is something up for discussion, and I can change to whatever desired. I have preliminary changed the project setting from gles2_use_nvidia_rect_flicker_workaround to use_nvidia_rect_flicker_workaround, to show how we might do it.

There is a trade off here between

  • backward / forward compatibility
  • preventing settings 'bloat'

With a change to the existing setting, this means that existing projects that use this option may see an extra (useless) version of the old setting, because it is still in their project.godot file. This may be confusing. In addition, changes to the new option will not reflect if they open the project in an older godot version.

Let me know opinions on this, it is easy to change.

@lawnjelly lawnjelly changed the title [WIP] Add Nvidia Workaround for GLES3 Add Nvidia Workaround for GLES3 May 7, 2020
@lawnjelly lawnjelly force-pushed the gles3_nvidia_workaround branch from 3aad68c to 327e3d3 Compare May 7, 2020 07:57
@akien-mga
Copy link
Member

With a change to the existing setting, this means that existing projects that use this option may see an extra (useless) version of the old setting, because it is still in their project.godot file. This may be confusing. In addition, changes to the new option will not reflect if they open the project in an older godot version.

This also means that if they previously enabled the option, it will no longer be enabled once they update to 3.2.2, so they will have to change this manually.

But if we make it crystal clear in the release notes, it shouldn't be a big issue. I don't expect that many users actually use this workaround in production due to the big performance hit that it gives.

@akien-mga
Copy link
Member

Needs a rebase after I merged your other PR.

@lawnjelly lawnjelly force-pushed the gles3_nvidia_workaround branch from 327e3d3 to c8e77a8 Compare May 7, 2020 13:24
@lawnjelly
Copy link
Member Author

Needs a rebase after I merged your other PR.

Done. 👍

use_nvidia_rect_workaround = false;

#ifdef GLES_OVER_GL
if (!Engine::get_singleton()->is_editor_hint()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why exclude it from the editor? Users might still see flickering in their TileMaps when editing them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally didn't, but the IDE seemed a little slower typing scripts with it set on in the editor. Maybe I'm imagining it. I'm ok with leaving it on in the editor too, I'll change this.

@lawnjelly lawnjelly force-pushed the gles3_nvidia_workaround branch from c8e77a8 to 6b18986 Compare May 7, 2020 13:50
Ported GLES2 workaround code to GLES3.
@lawnjelly lawnjelly force-pushed the gles3_nvidia_workaround branch from 6b18986 to dcb19ed Compare May 7, 2020 13:55
@Duroxxigar
Copy link
Contributor

@akien-mga Unfortunately, I have to use it for my 2D games. The flickering is gosh awful and WAYYYY too frequent to allow it in a released title. It's one of the main reasons I don't make 2D games that much these days in Godot.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks fine to me.

@akien-mga
Copy link
Member

This also means that if they previously enabled the option, it will no longer be enabled once they update to 3.2.2, so they will have to change this manually.

But if we make it crystal clear in the release notes, it shouldn't be a big issue.

Moreover, the Nvidia workaround is now only used when batching is OFF, and by default in 3.2.2 batching will be ON, so IMO it's OK that we also rename the setting for clarity and to avoid duplication.

If some users decide to disable batching, and they used to need the Nvidia workaround, they'll see the new one next to the use_batching setting.

@akien-mga akien-mga merged commit d038a7e into godotengine:3.2 May 8, 2020
@akien-mga
Copy link
Member

Thanks!

@lawnjelly lawnjelly deleted the gles3_nvidia_workaround branch May 8, 2020 09:02
@paulhocker
Copy link

@lawnjelly I can confirm with m,y build using your code fix has corrected the flicker problem. I tried both with and without the setting. Without the setting, the flicker was there (expected). With the setting, the flickering disappeared.

I did notice that the flickering still occurred in the Editor?

@lawnjelly
Copy link
Member Author

I did notice that the flickering still occurred in the Editor?

There may actually be a couple of places in the editor that draw using texture rect from a different place. I noticed it while working on the shader today. I'll have a look at changing those too, however first of all I need to get you (or someone else) to test out #38628, if you have any time to try it.

If that PR works, we can get rid of the workarounds completely, and you can run things at full speed, which would be much better! 👍

@paulhocker
Copy link

gotcha -- i'll try to find some time today to test that out for you.

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.

6 participants