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 a nearest-neighbor scaling option to Viewport's Scaling 3D Mode property #79731

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

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jul 20, 2023

This is useful for 3D games with a pixel art appearance, or when using a resolution scale of 0.5 to improve performance without compromising crispness too much when not using FSR 1.0.

The property hints now allow decreasing the scale further to accomodate for pixel art use cases, as well as increased precision in the value (useful for a scale of 0.3333).

This is supported in all rendering methods.

I've made sure that effects relying on bilinear filtering to look correct (such as glow and depth of field) still work correctly. No Vulkan validation errors are reported when running with --gpu-validation on either rendering method.

Testing project: test_scaling_3d_nearest.zip

Preview

Bilinear

Screenshot_20230721_014809

AMD FSR 1.0

Screenshot_20230721_014756

Nearest (this PR)

Screenshot_20230721_014711

@Calinou Calinou added this to the 4.x milestone Jul 20, 2023
@Calinou Calinou requested review from a team as code owners July 20, 2023 23:58
@Sauermann
Copy link
Contributor

I 'm not familiar with RenderingServer, but would this help with #72548?

@Calinou
Copy link
Member Author

Calinou commented Jul 21, 2023

I 'm not familiar with RenderingServer, but would this help with #72548?

Not entirely, as the use case from that issue is to scale both the 2D and 3D rendering. We only have scaling_3d_scale, not scaling_2d_scale 🙂

@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from 686201d to a36519a Compare September 25, 2023 22:01
@Calinou
Copy link
Member Author

Calinou commented Sep 25, 2023

Rebased and tested again, it works as expected.

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Sep 28, 2023
@akien-mga
Copy link
Member

CC @DarioSamo

@DarioSamo
Copy link
Contributor

DarioSamo commented Oct 2, 2023

Code looks fine to me, although I'm not the biggest fan of nearest due to the shimmering on uneven pixel scales like the documentation points out. It makes sense to provide the option regardless.

For games that actually want to preserve the pixel art aspect, I'm a bigger fan of filters like these, although they're not free performance wise compared to nearest.

But like I said this is one case where the more options the better, as nearest will work properly if the developer makes sure the resolution scale is accurate.

@clayjohn clayjohn modified the milestones: 4.2, 4.3 Oct 25, 2023
@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from a36519a to 540b914 Compare October 25, 2023 18:50
@mrjustaguy
Copy link
Contributor

What happens if you set resolution scale above 1 with nearest scaling? such as 2?
In such scenarios I can see it breaking and might be better to check for that case and revert to bilinear

@Calinou
Copy link
Member Author

Calinou commented Nov 28, 2023

What happens if you set resolution scale above 1 with nearest scaling? such as 2? In such scenarios I can see it breaking and might be better to check for that case and revert to bilinear

It reverts to bilinear automatically if resolution scale is above 1.0: https://github.com/godotengine/godot/pull/79731/files#diff-48a2a86e07288d8aa1500bb912bd1d7e51477df56d3a0660648b2382c4a79a4bR136-R139

@squishyu
Copy link

squishyu commented May 2, 2024

Any updates on merging this? Just saw v4.3 is almost in beta, would be nice to finally add this.

@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from 540b914 to f8d16e4 Compare May 2, 2024 20:52
@Calinou
Copy link
Member Author

Calinou commented May 2, 2024

Rebased and tested again, it works as expected in Forward+ and Mobile.

Note that 3D scaling support was implemented in the Compatibility rendering method in #83976, but I haven't implemented nearest-neighbor filtering there yet.

@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from f8d16e4 to ffe9ec6 Compare May 9, 2024 03:33
@Calinou
Copy link
Member Author

Calinou commented May 9, 2024

It's implemented in Compatibility now:

Bilinear

godot windows editor x86_64_5RbHuxXRVt

Nearest

godot windows editor x86_64_YfK94QxbKT

@GlitchedPolygons

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

AThousandShips commented May 20, 2024

@GlitchedPolygons Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

Edit: Add your reaction to the main post at the very top, or add your vote here (do not comment unless you have new information there either):

@GlitchedPolygons
Copy link

@GlitchedPolygons Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

sry, I'm new here and I got excited with this PR :)

is there anywhere else where I can add my vote for this?

Copy link
Contributor

@radiantgurl radiantgurl left a comment

Choose a reason for hiding this comment

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

Tested, working as expected.

RD::Uniform u_source_color;
u_source_color.uniform_type = RD::UNIFORM_TYPE_INPUT_ATTACHMENT;
u_source_color.binding = 0;
u_source_color.append_id(viewport_scale_sampler);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about this, you are passing in a sampler, but its in an input attachment, not a UNIFORM_TYPE_SAMPLER_WITH_TEXTURE. Additionally, clearly this was working previously without a linear sampler, and yet there are no shader changes to accommodate the sampler.

How does this work?

Copy link
Member Author

@Calinou Calinou Dec 11, 2024

Choose a reason for hiding this comment

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

I don't know either why this is working. I'll need to test under D3D12 and Metal to make sure it works there as well.

--gpu-validation doesn't print any warnings related to this under Vulkan.

Copy link
Member

Choose a reason for hiding this comment

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

I think if this is using a texture and sampler then it should use the texture + sampler uniform type instead of the input attachment uniform type

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've changed it to RD::UNIFORM_TYPE_SAMPLER_WITH_TEXTURE and can confirm it still works.

@Framebuffers
Copy link

Hi, I did a game with this PR baked in for a game jam. The game needed this kind of look, so I self-compiled the engine with this PR baked into the engine. Its code is in my GitHub as Premonition.

On that repo's description there's a link to the binary on itch.io. The game itself is written in C#, and the code isn't good at all. However, it's a good tech demo of how the PR would look like on a released game.

Hope it helps, and thanks @Calinou for your hard work on the engine! Really hope I can contribute little by little and give back to this wonderful community.

@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from ffe9ec6 to 88bfd73 Compare January 7, 2025 00:01
@Calinou
Copy link
Member Author

Calinou commented Jan 7, 2025

Rebased and tested again following the merge of #99603, it works as expected.

Note that if you were already using this PR locally, you'll have to reassign the Scaling 3D Mode project setting and/or Viewport property as the enum values were shifted around (Nearest is now at 5 instead of 3 to accomodate for the MetalFX options being placed before it). The enum's ordering in the project setting was changed so that Nearest is displayed first, but Bilinear remains the default. (This does not affect the actual enum values, it's an editor-only change using property hints.)

@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from 88bfd73 to 6303f7f Compare January 7, 2025 00:05
@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch 2 times, most recently from 7b87b9b to 9087e1b Compare January 7, 2025 16:10
…roperty

This is useful for 3D games with a pixel art appearance, or when
using a resolution scale of `0.5` to improve performance without
compromising crispness too much when not using FSR 1.0.

The property hints now allow decreasing the scale further to accomodate
for pixel art use cases, as well as increased precision in the value
(useful for a scale of `0.3333`).
@Calinou Calinou force-pushed the viewport-add-nearest-3d-scaling branch from 9087e1b to e8ca7c0 Compare January 25, 2025 11:06
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.

Add a nearest-neighbor scaling option to Viewport's Scaling 3D Mode property