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

Fix blurred minimap #1897

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bcdrme
Copy link
Contributor

@bcdrme bcdrme commented Jan 14, 2025

The minimap clarity and sharpness shouldn't be impacted by AA settings.
Turns out the texture was also linearly filtered.

This PR

  • removes AA entirely from the minimap rendering
  • changes the Linear filtering to a Nearest filtering

Here is a screenshot of the issue
Screenshot From 2025-01-13 21-35-44

@bcdrme bcdrme force-pushed the fix-blurred-minimap branch from e517e43 to e5230b9 Compare January 14, 2025 12:06
@sprunk
Copy link
Collaborator

sprunk commented Jan 14, 2025

What about fullscreen minimap (for dual screen), won't this make it ugly? You can probably test this by just rescaling the minimap to be huge on a single screen as well.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 14, 2025

What about fullscreen minimap (for dual screen), won't this make it ugly? You can probably test this by just rescaling the minimap to be huge on a single screen as well.

It looks very clean and sharp to me, I would argue it's an improvement even.

Hopefully the image compression gives it justice
image

Copy link
Collaborator

@sprunk sprunk left a comment

Choose a reason for hiding this comment

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

What about maps with some sort of checkerboard or otherwise repeating patterns? Think Kernel Panic maps or OG speedmetal.

What about 1px items (think projectiles), won't they disappear?

Making icons not subject to blurring but keeping everything else as-is sounds best.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 14, 2025

What about maps with some sort of checkerboard or otherwise repeating patterns? Think Kernel Panic maps or OG speedmetal.

What about 1px items (think projectiles), won't they disappear?

I don't see any issue with either the checkerboard pattern nor the projectiles.

Making icons not subject to blurring but keeping everything else as-is sounds best.

It might sounds best but it's arguably not. Especially for very small things like lasers and projectiles.
I suggest you try this change for yourself in game, see how it feels

@lhog
Copy link
Collaborator

lhog commented Jan 15, 2025

screen_2025-01-15_21-02-46-955
screen_2025-01-15_21-05-25-247

I don't see any visual difference between the master and your branch.
Even if it was the code has a lot of redundant fbo.Bind() calls, which are potentially expensive.
Can you tell why you removed the rendering into MSAA fbo/renderbuffer?

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 15, 2025

Do you have AA enabled ?
The issue is particularly obvious with SSAA, if you want to see worst case scenario you can run the engine with

MSAA = 1
MSAALevel = 8
MinSampleShadingRate = 1.0

Removing MSAA fbo/renderbuffer fixes the blurriness induced by enabling SSAA.
Moving to GL_NEAREST finishes the job in terms of readability, especially when the minimap is rendered on a small area (say 200x200 pixels).

This is how it looks best imho.

Your test doesn't seem to run in the context of BAR, how can I run this myself ?

@lhog
Copy link
Collaborator

lhog commented Jan 15, 2025

MSAALevel = 4 for me, MSAA = 1 doesn't do anything, MinSampleShadingRate = 1.0 is not really practical for anything but screenshots: it makes model edges look a bit better and fonts - sharper, but these constitute for like 5% of the screen space, while you pay full shading price for 100% of pixels.

Overall GL_LINEAR->GL_NEAREST replacement might be fine, but the MSAA renderbuffer removal is very questionable.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 15, 2025

MinSampleShadingRate = 1.0 is not really practical for anything but screenshots: it makes model edges look a bit better and fonts - sharper, but these constitute for like 5% of the screen space, while you pay full shading price for 100% of pixels.

There's a significant difference in image quality with MinSampleShadingRate = 1.0, very noticeable on my big screen.
I play with these settings on, this is not only for screenshots... I don't see how this invalidates this PR.

but the MSAA renderbuffer removal is very questionable.

Can you elaborate why ?

@lhog
Copy link
Collaborator

lhog commented Jan 15, 2025

Can you elaborate why ?

Because this way you disable MSAA for minimap texture. How is that a good thing?

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 15, 2025

It's a good thing in my opinion because we are rendering very small thing on a very small area. And any amount of smoothing on this texture hurts the readability of the minimap. Now lasers, projectiles and icons are way more readable.

Now,

  • GL_LINEAR has the biggest destructive effect.
  • MSAA comes next.

I have generated another set of screenshots to illustrate my argument in two different sizes, in a real scenario.
I wish I could take uncompressed screenshots so that it's more obvious but I hope this will do:

Screenshots are presented in this order :

  1. GL_LINEAR + MSAA
  2. GL_NEAREST + MSAA
  3. GL_NEAREST

Screenshot From 2025-01-15 23-03-35
Screenshot From 2025-01-15 23-21-58
Screenshot From 2025-01-15 23-15-19

Again, same with bigger minimap

  1. GL_LINEAR + MSAA
  2. GL_NEAREST + MSAA
  3. GL_NEAREST

Screenshot From 2025-01-15 23-03-05
Screenshot From 2025-01-15 23-21-17
Screenshot From 2025-01-15 23-16-10

@lhog
Copy link
Collaborator

lhog commented Jan 15, 2025

See if you can repro the same result with engine's minimap drawing in https://github.com/DarkBlueDiamond/VroomRTS/ or just with LuaUI disabled.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 15, 2025

Will do 👍

Note: I could live with only the GL_NEAREST change being accepted as it is a big improvement already.

Having a setting that allows you to disable AA on the minimap would be the best thing since I can see that being subjective and down to preference.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 16, 2025

See if you can repro the same result with engine's minimap drawing in https://github.com/DarkBlueDiamond/VroomRTS/ or just with LuaUI disabled.

100% reproducible with /luaui disable.

  1. GL_LINEAR + MSAA
  2. GL_NEAREST + MSAA
  3. GL_NEAREST

image
Screenshot From 2025-01-16 17-27-26
Screenshot From 2025-01-16 17-33-06

I think a good analogy for minimaps is pixel art honestly. You don't want any filtering on your pixel art otherwise it looks like a mess.

@lhog
Copy link
Collaborator

lhog commented Jan 16, 2025

That shouldn't matter if only slightest, but did you have non-zero MinSampleShadingRate when you took these screenshots?
Somehow I don't see what you see.

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 16, 2025

MinSampleShadingRate = 1.0 yeah, sorry I should have mentioned it more clearly.

Without that, the minimap stays unfiltered even at max MSAALevel.
It's also true for

  • text in general (UI, player names, loading screen)
  • RmlUi

Maybe this highlights a more global issue with MSAA.
Why would text remains aliased up until you set MinSampleShadingRate to 1 for example ?
🤔

@lhog
Copy link
Collaborator

lhog commented Jan 16, 2025

Ok, I'll have a deeper look later.
In the meantime what if you change to GL_NEAREST here https://github.com/beyond-all-reason/spring/blob/master/rts/Game/UI/MiniMap.cpp#L1152 ?

@bcdrme
Copy link
Contributor Author

bcdrme commented Jan 16, 2025

Ok, I'll have a deeper look later. In the meantime what if you change to GL_NEAREST here https://github.com/beyond-all-reason/spring/blob/master/rts/Game/UI/MiniMap.cpp#L1152 ?

I tested different scenarios

  • only this change
  • NEAREST here and on the other two lines
  • NEAREST here and on the other two lines and multisampledFBO force to false

It doesn't looks like it has an impact on the final result.

Left: NEAREST here and on the other two lines + AA
Right: NEAREST + AA removed (multisampledFBO forced to false in the original code)

Screenshot From 2025-01-16 19-18-41

@bcdrme bcdrme force-pushed the fix-blurred-minimap branch from e5230b9 to 4a7dd38 Compare January 19, 2025 19:27
@bcdrme bcdrme force-pushed the fix-blurred-minimap branch from 4a7dd38 to 0ba3bb8 Compare January 19, 2025 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants