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

Implement "Battlefield Camera" setting and smooth zooming #17424

Merged
merged 4 commits into from Dec 13, 2019

Conversation

@pchote
Copy link
Member

pchote commented Dec 4, 2019

This PR builds on #17308 and its prerequisites to finally implement the world "camera height" / custom resolution setting outlined in #10382. This also adds smooth mouse-wheel zooming and makes several changes to the zoom hotkeys and configuration to fit with the new and next changes.

A key part of this PR is an implementation of a custom scaling filter (inspired by this article) that resamples the world frame buffer without excessive blurring or artifacting.

The focus for this PR is to support players (myself included) who have monitors where the native scaling is uncomfortably small, but pixel doubling is uncomfortably large. I've tried to take into account the feedback from the Discord discussion I started a few months ago, and think I've found a good solution that improves the default configuration for many players while giving people who prefer the old behaviour ways to preserve it.

There are several scope creeps that I considered but ruled out from implementing in this PR:

  • Supporting zooms < 1, aside from keeping compatibility with the existing ability for spectators and the map editor - this requires deeper changes to the viewport scrolling code to work well, and opens up a can of wormy gameplay implications. More discussion and experimentation is required.
  • Supporting zoom ranges more than just 1-2 - this requires deeper thought and community relations wrt the impact on the toggle zoom (now reset zoom) hotkey and breaking/changing muscle memory.
  • Allowing missions / lobby options to restrict the maximum viewport size - this one should be fairly safe, but is better as its own PR.
  • Implementing the UI scaling half of #10382 - this is another large pile of work, which I will be continuing with in the next set of PRs.
  • Scaling health bars etc - this is part of the UI scaling, above.

Depends on #17308, #17431, #17435.

People on Windows with their DPI/desktop scaling not at 100% may need to disable Graphics.DisableWindowsDPIScaling to see the proper effect of this PR. This setting will be removed as part of the UI scaling overhaul.

@pchote pchote added this to the Next+1 milestone Dec 4, 2019
OpenRA.Game/Settings.cs Show resolved Hide resolved
OpenRA.Game/Settings.cs Show resolved Hide resolved
@@ -184,14 +184,20 @@ void RegisterSettingsPanel(PanelType type, Func<Widget, Action> init, Func<Widge
resetPanelActions.Add(type, reset(panel));
}

static readonly Dictionary<WorldViewport, string> ViewportSizeNames = new Dictionary<WorldViewport, string>()
{
{ WorldViewport.Close, "Close" },

This comment has been minimized.

Copy link
@pchote

pchote Dec 4, 2019

Author Member

Close is ~windows 95 game size (with the 2x zoom then being the ~dos size)
Medium is ~720p
Far is ~1080p
Native/Furthest is always a zoom of 1.

The "~" is because the game will try to pick a size that is near but not necessarily matching these so we don't unnecessarily degrade the visual quality at different aspect ratios.

mods/common/chrome/editor.yaml Show resolved Hide resolved
@pchote pchote force-pushed the pchote:render-zoom branch from 1c30634 to 910656d Dec 5, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 5, 2019

Rebased and fixed a shader compilation error on non-macOS.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 5, 2019

This looks really great in general.

Some issues I noticed while testing:

  • Sometimes when spectating in TS at the closest setting I get some weird artifacts: Black jagged blocks at the bottom of the screen. It seems to be map dependent; I get it consistently on Tactical.
  • I think the scroll zoom could benefit from having an acceleration curve.
  • At the closest setting there is a noticeable wobble when zooming.
OpenRA.Game/Graphics/Viewport.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 5, 2019

Sometimes when spectating in TS at the closest setting I get some weird artifacts: Black jagged blocks at the bottom of the screen. It seems to be map dependent; I get it consistently on Tactical.

Can you please post a screenshot? I suspect this will be a regression from https://github.com/OpenRA/OpenRA/pull/17308/files#diff-000122b57c1834317a240a39ed02beabR256 in #17308, but I can't reproduce the issue locally.

I think the scroll zoom could benefit from having an acceleration curve.

Sounds reasonable, but I don't have the interest to experiment with values for this. If someone can provide calibration values i'll add them, otherwise we should defer this to a future PR (or if its a blocker I'll drop the smooth zooming from this PR - its a nice extra, but not my focus here).

At the closest setting there is a noticeable wobble when zooming.

This is caused by the pixel rounding in the viewport scrolling that requires a deeper rework of the Viewport code to fix. As above, my solution will be to drop the scroll zooming if this is considered a blocker.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 5, 2019

Screenshot from 2019-12-05 22-08-27

Disregard the colored strip in the middle; that is just a glitch of my OS when making screenshots. It is the black triangles at the bottom.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 5, 2019

Can you confirm that reverting the / 2 in the GetScissorBounds change I linked above fixes the issue?

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 5, 2019

Reverting that doesn't fix it.

I can't reproduce the issue locally.

It seems not only map, but also location dependent. It happens for me when I have the camera around the middle spawn position on Tactical.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 5, 2019

Thanks, I can reproduce it now. The problem happens when zooming in by more than a specific threshold on terrain that has a height offset of more than 10 steps. I haven't yet found whatever part of the code this breaks.

Edit: The underlying issue seems to be a regression from #16996. The issue appears (on my screen - trigger point may be different for other resolutions) at a zoom of 5.6, where the worldBufferSize in Renderer switches from 512,512 to 512,256. This seems to mess up some coordinate scaling, breaking the debug overlays and the depth buffer - the latter seems to cause the tile discarding.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 5, 2019

There are two issues here:

  • Zooming to large values breaks this assumption.
  • Zooming to non-power-of-two scales breaks the assumption that worldBufferSize and surfaceBufferSize have the same aspect ratio relative to their backing sheet, which breaks the rendering of annotation renderables (best seen for for voxels with the render debug overlay enabled).

These may want to be fixed in a separate PR (once we work out how), but this one does provide a good testcase.

@pchote pchote force-pushed the pchote:render-zoom branch from 910656d to b361452 Dec 6, 2019
@pchote pchote force-pushed the pchote:render-zoom branch from b361452 to 048b076 Dec 6, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 6, 2019

Updated:

  • Fixed the jagged-bottom-edge issue by rebasing on the updated #17308
  • Changed unlockMinimumZoomScale definition
  • Added zoom hotkeys: [ and ] because - is already used and IMO it is awkward for zooming in to require a modifier but zooming out not (at least on my keyboard)
@pchote pchote force-pushed the pchote:render-zoom branch from 048b076 to 6dcc683 Dec 6, 2019
@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 6, 2019

I can confirm the jagged bottom edge is fixed.

Regarding the wobble: The same wobble is present in the RA shell-map as the camera moves around. It is especially noticeable when setting the camera to "close". Strangely, there is no wobble present when you manually scroll the camera around the map, not even when you do it very slowly using the "joystick" configuration.

it is awkward for zooming in to require a modifier but zooming out not (at least on my keyboard)

Yeah, realized that after commenting. I guess I was thinking of the numpad + and - keys, but many laptops don't have a numpad so that is not a helpful default.

Regarding the acceleration curve: I've found this article, which provides a reasonable looking curve:
https://www.microsoft.com/en-us/research/wp-content/uploads/2016/02/Scrolling_CHI02.pdf
However, it seems we need to make some slight changes to how we register scroll wheel movement in order to be able to apply a curve. We will need to know the time between subsequent notches of the wheel, which we currently don't track.

That said, the lack of acceleration may not be the main reason why scrolling feels a bit off. I noticed that scroll zooming gets slower the more zoomed in you are. This is not even relative to the base zoom level. For example, with the scroll speed slider somewhere in the middle, it took me 16 notches to get from 1x to 2x on 'furthest'. Yet on 'close' it took me 49 notches to go from 1x to 2x!

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 6, 2019

For example, with the scroll speed slider somewhere in the middle, it took me 16 notches to get from 1x to 2x on 'furthest'. Yet on 'close' it took me 49 notches to go from 1x to 2x!

This is because scrolling adds/subtracts absolute deltas from the zoom: at furthest/native zoom the range between min and max zoom will be 1 (1-2). At the closest zoom we may end up scrolling between 3 and 6, which requires 3 times as many steps.

We need to be scaling the deltas by the base zoom amount, like I just implemented for the hotkeys.

@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 7, 2019

That still wouldn't be quite right. That makes it consistent between the different base zoom settings, but the spacing between the notches is still off. This is especially notable in the map editor where the zoom range is larger. The spacing between the hotkey steps is similarly off.

Fortunately, this can be easily remedied. I locally replaced the linear scale with an exponential scale and that completely solved it.

i.e. inside AdjustZoom replace

Zoom = (zoom + dz).Clamp(unlockMinZoom ? unlockedMinZoom : minZoom, maxZoom);

with

Zoom = (zoom * (float)Math.Exp(dz)).Clamp(unlockMinZoom ? unlockedMinZoom : minZoom, maxZoom);

This also makes scaling by the base zoom amount redundant.

@pchote pchote force-pushed the pchote:render-zoom branch from 6dcc683 to 04d5f4b Dec 7, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 7, 2019

Updated:

  • Fixed zoom scaling
  • Fixed map editor glitches by rebasing on #17431.
@tovl

This comment has been minimized.

Copy link
Contributor

tovl commented Dec 7, 2019

For the record: With the new zoom scaling I don't consider scroll wheel acceleration within the scope of this PR anymore. The wobble in the zoom is also a lot less noticeable with the new scaling, so I don't consider that a blocker. The wobble in the RA shellmap seems to be present on bleed as well, it is just more noticeable with a close camera. Therefore, that is probably out-of-scope as well.

As for the scaling filter itself: I don't have much to say here because it seems to work perfectly. I didn't notice any visual artifacts on any arbitrary zoom level > 1. With zoom < 1 there seems to be some aliasing when moving the map, but that only becomes bothersome with zoom < 0.5 (so only in editor mode; not in spectator mode).

In fact, I couldn't tell any difference in visual fidelity between the optimized default zoom levels and the arbitrary intermediate scroll zoom levels. So I'm beginning to wonder if we really need to have those approximate window heights and wouldn't be better off just using fixed window heights as presets.

Just one more nit: "reset zoom" should be listed under viewport commands, not game commands.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 9, 2019

Added a commit to fix the GPS dot rendering glitch discovered by @netnazgul.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 10, 2019

Some additional notes on zooming, possibly out of scope for this.

  1. Quite accustomed to have "zoom-to-cursor" feature where viewport also centers to mouse pointer during zoom rather than the screen center.
  2. Without inertial zoom 90% of values for "zoom speed" are mostly unusable because you need to mash the scroll quite a number of times to get from 0.5x to 2x. I think even on max setting you can't scroll in/out in a single wheel movement.
@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Dec 11, 2019

Needs a rebase!

@pchote pchote force-pushed the pchote:render-zoom branch from 5e4ad5f to b6420c0 Dec 11, 2019
@pchote pchote removed the PR: Rebase me! label Dec 11, 2019
@pchote pchote force-pushed the pchote:render-zoom branch from b6420c0 to dd163be Dec 11, 2019
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 11, 2019

Updated:

  • Rebased
  • Restored mouse wheel zoom modifier and removed the Scroll Zooming checkbox instead
  • Doubled maximum zoom speed
  • Adjusted settings menu layout
@pchote pchote force-pushed the pchote:render-zoom branch from dd163be to 6927e7e Dec 12, 2019
@tovl
tovl approved these changes Dec 13, 2019
@tovl tovl merged commit 28dbda2 into OpenRA:bleed Dec 13, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 13, 2019

@pchote pchote deleted the pchote:render-zoom branch Dec 13, 2019
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 14, 2019

This introduced the following crash when I force GL ES on my GPU (GeForce GTX 750 on Win10, driver version 432.00):

GL Info Log:
0(88) : error C7011: implicit cast from "int" to "float"
0(90) : error C7011: implicit cast from "ivec2" to "vec2"

System.InvalidProgramException: Compile error in shader object 'D:\OpenRA-AS-dev\glsl\combined.frag'
   at OpenRA.Platforms.Default.Shader.CompileShaderObject(Int32 type, String name)
   at OpenRA.Platforms.Default.Shader..ctor(String name)
   at OpenRA.Platforms.Default.Sdl2GraphicsContext.CreateShader(String name)
   at OpenRA.Renderer..ctor(IPlatform platform, GraphicSettings graphicSettings)
   at OpenRA.Game.Initialize(Arguments args)
GL Info Log:
0(88) : error C7011: implicit cast from "int" to "float"
0(90) : error C7011: implicit cast from "ivec2" to "vec2"

System.InvalidProgramException: Compile error in shader object 'D:\OpenRA-AS-dev\glsl\combined.frag'
   at OpenRA.Platforms.Default.Shader.CompileShaderObject(Int32 type, String name)
   at OpenRA.Platforms.Default.Shader..ctor(String name)
   at OpenRA.Platforms.Default.Sdl2GraphicsContext.CreateShader(String name)
   at OpenRA.Renderer..ctor(IPlatform platform, GraphicSettings graphicSettings)
   at OpenRA.Game.Initialize(Arguments args)
@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 15, 2019

Is this sufficient to fix it?

diff --git a/glsl/combined.frag b/glsl/combined.frag
index 773386be93..0288f763bc 100644
--- a/glsl/combined.frag
+++ b/glsl/combined.frag
@@ -85,9 +85,9 @@ void main()
 {
        vec2 coords = vTexCoord.st;
 
-       if (AntialiasPixelsPerTexel > 0)
+       if (AntialiasPixelsPerTexel > 0.0)
        {
-               vec2 textureSize = Size(vTexSampler.s);
+               ivec2 textureSize = Size(vTexSampler.s);
                vec2 offset = fract(coords.st * textureSize);
 
                // Offset the sampling point to simulate bilinear intepolation in window coordinates instead of texture coordinates
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 15, 2019

Sadly not. That then breaks L91 and L99 with the same error message. Changing L91 just further generates errors, breaking L98 and L99 multiple times even, with a float-int conversion along.

I'll disable GL ES for now instead - I've had it enabled for testing #17087.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Dec 15, 2019

Can you please test with

diff --git a/glsl/combined.frag b/glsl/combined.frag
index 773386be93..2b63063c91 100644
--- a/glsl/combined.frag
+++ b/glsl/combined.frag
@@ -85,9 +85,9 @@ void main()
 {
        vec2 coords = vTexCoord.st;
 
-       if (AntialiasPixelsPerTexel > 0)
+       if (AntialiasPixelsPerTexel > 0.0)
        {
-               vec2 textureSize = Size(vTexSampler.s);
+               vec2 textureSize = vec2(Size(vTexSampler.s));
                vec2 offset = fract(coords.st * textureSize);
 
                // Offset the sampling point to simulate bilinear intepolation in window coordinates instead of texture coordinates

?

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Dec 15, 2019

Bingo! That one loads.

public readonly int2 FarWindowHeights = new int2(900, 1300);

public readonly float MaxZoomScale = 2.0f;
public readonly int MaxZoomWindowHeight = 240;

This comment has been minimized.

Copy link
@Mailaender

Mailaender Jan 12, 2020

Member

It would be great to have [Desc() documentation on these as they are never used and not quite self explanatory.

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.