OpenArena / engine

OpenArena modifications to the ioquake3 engine
http://openarena.ws
GNU General Public License v2.0
212 stars 50 forks source link

Changed widescreen FOV formula to Hor+ #37

Closed ryvnf closed 5 years ago

ryvnf commented 6 years ago

I noticed that switching to the new engine broke my zoom sensitivity config that uses the tricks described here

After some investigation I found that the problem was the widescreen support code that calculates new FOV values. I don't know exactly how the previous formula calculated the new FOV values. But I found that it does work for 90 degree FOV, but gives slightly incorrect result for other FOV values.

Hor+ is the formula most commonly used in games. In that formula the vertical FOV stays the same as if you where playing on 4:3 monitor while the horizontal FOV is expanded to fit the screen. Switching to this formula makes the FOV values consistent with that is used in most other games.

Fixing this is especially important for this change in the gamecode to work correctly.

I also found the new FOV calculation code to be more readable which is a plus.

ryvnf commented 6 years ago

Just noticed my change has a slight bug where the main menu OpenArena logo gets scaled (so it becomes quite small). I will try to fix it as soon as possible.

I have verified that it works as expected in game though.

ryvnf commented 6 years ago

I solved that problem by adding another commit. Now it doesn't affect the FOV-values in menus.

The-Gig commented 6 years ago

About "previous formula": scrolling back engine commits history, I noticed a couple of commit descriptions mentioning widescreen or horz+, maybe they may help: https://github.com/OpenArena/engine/commit/9d43ff7e6c2fa5cec24df4654dbc39510fc11240 https://github.com/OpenArena/engine/commit/1947afbe2e2b1326bd577081b544d5ba55e72cf8 There might be more...

(AFAIK, while OA up to 0.8.8 binaries used VERT-, the code in GIT repository switched to HOR+ since a few years ago. I can't tell if there is some error in such implementation.)

ryvnf commented 6 years ago

As said the current widescreen FOV formula implemented by leilei works as it should for cg_fov 90. The problem is that it doesn't work as it should for other cg_fov values. The difference is illustrated by the following 2 pictures.

newold-engine-zoom patchedold-engine-zoom

Each picture is 2 screenshots drawn on top of each other. I used the opposite sides of the map ps37ctf so it is obvious by the color. The locations are exactly the same in both screenshots. One of the pictures is drawn using the old Vert- engine with cg_fov 57.822 which mathematically is equivalent to 45 Hor+ FOV at 16:9. It is compared with the current engine (leilei's hor+) in the upper picture. You see that the images misalign quite a bit.

The lower image is cg_fov 45 with the patched engine (that I created this pull request for) compared to the old engine with 57.822 FOV. As you can see the images align perfectly and the red and blue mixes and becomes purple.

I see no reason for this change to not be accepted. It is a minor but very important bug-fix. The new FOV calculation code is also much simpler and easier to understand.

The-Gig commented 6 years ago

ryvnf, did you do some benchmark test (e.g. timedemo) to see if your code runs faster or slower than Leilei's code?

ryvnf commented 6 years ago

I have not done any benchmarks. But any difference in performance would be unnoticeable.

The new code has the same number of calls to tan and atan2, but it also shorter and has less other things going on (see the diff), so if there would be any difference the new version would probably be faster.

And as original code is bugged, I don't think it makes much sense to compare their performance.

Kezuz commented 5 years ago

@ryvnf, you are correct that the current implementation is flawed. I tested it myself with 130 degree FOV:

130

The vertical fov changes between 640x480, 800x480 and 960x480 which it shouldn't in a correct implementation.

Kezuz commented 5 years ago

Compare that with the result of this pull request:

ryvnf