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

Q2pro updates #166

Merged
merged 357 commits into from
Dec 18, 2021
Merged

Q2pro updates #166

merged 357 commits into from
Dec 18, 2021

Conversation

Paril
Copy link
Contributor

@Paril Paril commented Dec 10, 2021

This branch pulls in all of the changes from Q2PRO between c3e4273d800fafbf50883f2a7b97eb7fd0161b74 and 59d0fc43ccecaab878889a2a33393faeab2e7dca (the last commit Skuller had made), bringing Q2RTX up to date with Q2PRO's upstream.

The majority of large changes were tested, but I've not validated that every little detail is correct. Some extra eyes would help.

skullernet and others added 30 commits December 10, 2021 00:05
Fixes rescan logic broken in d430e36.
From: Andrey Nazarov <[email protected]>
Date: Wed, 2 May 2018 23:54:49 +0300
Subject: [PATCH 122/396] Remove SDL 1 support.
From: Andrey Nazarov <[email protected]>
Date: Thu, 3 May 2018 00:06:28 +0300
Subject: [PATCH 123/396] Remove CONFIG_DIRECT_INPUT support.
From: Andrey Nazarov <[email protected]>
Date: Thu, 3 May 2018 00:09:53 +0300
Subject: [PATCH 124/396] Remove CONFIG_LIRC support.
…to a DLL-based renderer subsystem again we can easily support software mode in the future, potentially using Yamagi's software renderer which is amazing.

From 8f2bd3109769fe6bb09c65aca29db2a8c184d17c Mon Sep 17 00:00:00 2001
From: Andrey Nazarov <[email protected]>
Date: Tue, 8 May 2018 18:05:46 +0300
Subject: [PATCH 125/396] Remove software renderer.
From: Andrey Nazarov <[email protected]>
Date: Sun, 27 May 2018 14:18:59 +0300
Subject: [PATCH 130/396] Fix SDL 2 subsystem init/shutdown.
From: Andrey Nazarov <[email protected]>
Date: Sun, 27 May 2018 16:14:13 +0300
Subject: [PATCH 131/396] Switch to the new QGL implementation.

Remove _ARB and _EXT suffixes from core functionality.
Remove GL calls logging.
Remove support for disabling use of buffer objects.
Remove CONFIG_FIXED_LIBGL support.
Use a single function to retrieve OpenGL entry points.
From: Andrey Nazarov <[email protected]>
Date: Sat, 2 Jun 2018 22:33:19 +0300
Subject: [PATCH 132/396] Add experimental GLSL rendering backend.

Implements lighting additions suggested by void-995 (see NVIDIA#144).
From: Andrey Nazarov <[email protected]>
Date: Sun, 3 Jun 2018 17:29:58 +0300
Subject: [PATCH 133/396] Remove commented out function.
Forgotten in 53d18b8.
Not a proper fix because add/modulate needs to be applied in the shader.
But who is going to use vertex lighting anyway?
Put the relevant parameters into registers before looping to help the
compiler produce better code.
From: Andrey Nazarov <[email protected]>
Date: Wed, 6 Jun 2018 16:44:34 +0300
Subject: [PATCH 138/396] Convert most GL version checks into capabilities.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also remove ‘gl_texture_non_power_of_two’ variable since it's not
useful.
From: Andrey Nazarov <[email protected]>
Date: Wed, 6 Jun 2018 16:47:04 +0300
Subject: [PATCH 139/396] Add generic GL extension blacklist support.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also remove explicit ‘gl_fragment_program’ variable since fragment
programs can now be disabled using generic blacklist.
VBOs are part of ES 1.1 core. Reject GL 1.0 to prevent crash since no
functions are going to be loaded. No point in enabling shaders on GL 2.0
hardware even if extensions allow it.
From: Andrey Nazarov <[email protected]>
Date: Wed, 6 Jun 2018 19:41:12 +0300
Subject: [PATCH 141/396] Enable GLSL rendering backend by default.
From: Andrey Nazarov <[email protected]>
Date: Fri, 8 Jun 2018 18:21:19 +0300
Subject: [PATCH 144/396] Fix building on systems with outdated GL headers.
skullernet and others added 18 commits December 10, 2021 00:07
libcurl messages are always "\n\0" terminated.
C99 conformant vsnprintf() is now required. Use standard modifier on all
platforms.
From: Andrey Nazarov <[email protected]>
Date: Sat, 11 Sep 2021 21:44:30 +0300
Subject: [PATCH 378/396] Use versionhelpers.h to check Windows version.

See NVIDIA#212.
Added win_noborder to client docs.
From: Andrey Nazarov <[email protected]>
Date: Fri, 17 Sep 2021 19:34:05 +0300
Subject: [PATCH 385/396] Remove non-working current thread check in exception
 filter.

This never worked as intended because GetCurrentThread() returns pseudo
handle relative to current thread.
From: Andrey Nazarov <[email protected]>
Date: Mon, 20 Sep 2021 15:12:21 +0300
Subject: [PATCH 388/396] Factor out function for header byteswapping.
From: Andrey Nazarov <[email protected]>
Date: Mon, 20 Sep 2021 19:41:42 +0300
Subject: [PATCH 389/396] Don't crash with fatal error if model hunk is
 exhausted.

Loading oversize MD2 or MD3 model can legitimately exhaust default 4 MiB
hunk and crash with fatal error.

Change Hunk_Alloc() to return NULL on allocation failure and add
specific checks, so that oversize model simply fails to load with
ENOMEM.

Note that all other users of Hunk_Alloc() already reserve enough memory,
thus no additional checks are needed.
libcurl handles are reused between downloads, make sure to set all
options so that old values are overwritten.
@res2k
Copy link
Contributor

res2k commented Dec 10, 2021

This is quite a large change with 355 commits. This is hard to review ... e.g. I don't think I have the stamina to do this.

However, just from a quick look, it seems the changes range from maintenance without semantic changes (like qboolean -> bool) to e.g. breaking changes (savegame format) - there would probably be different considerations for merging these changes (or not).
My recommendation would be to split the PR up into multiple smaller ones, perhaps with some grouping of "related" changes. This would simplify the reviewing the changes (and whether everything was correctly adapted for Q2RTX), making things a bit safer, at the cost of taking longer.

Disclaimer: this is all just my personal opinion, I don't decide what gets merged or not.

@Paril
Copy link
Contributor Author

Paril commented Dec 10, 2021

This is quite a large change with 355 commits. This is hard to review ... e.g. I don't think I have the stamina to do this.

However, just from a quick look, it seems the changes range from maintenance without semantic changes (like qboolean -> bool) to e.g. breaking changes (savegame format) - there would probably be different considerations for merging these changes (or not). My recommendation would be to split the PR up into multiple smaller ones, perhaps with some grouping of "related" changes. This would simplify the reviewing the changes (and whether everything was correctly adapted for Q2RTX), making things a bit safer, at the cost of taking longer.

Disclaimer: this is all just my personal opinion, I don't decide what gets merged or not.

If I recall the only breaking change was the save format, because of the qboolean changes & float -> int frametimes. Everything else is maintenance that Q2PRO was doing along the way - various changes to error reporting, asynchronous screenshots, and a lot of simplification (hence the net negative in total code). The OpenGL backend was also overhauled quite heavily, but luckily since Q2RTX hadn't touched any of it it was (mostly) cleanly patched. The legacy SDL backend was removed, as well as some other systems that Skuller probably decided weren't being used at all. The most trouble I ran into (in terms of differences) was with screenshotting, since that entire image system uses stbi in RTX, but it was simple to adapt to Q2PRO's async queue.

The problem with splitting up the PR is that nearly all of these changes are chronologically required, or depend on each other in some way. It would take me a lot longer to figure out the relationship between all of the commits than it was to adapt these as-is. I don't know if I have the stamina to do that, heh.

I took care in ensuring nothing was lost in the transitions; a lot of the more important commits were already merged along the way before this, luckily. Commit 61c1eab is probably the one that touches the most files, so if you're somehow able to diff the range excluding changes made by that commit it should be a bit easier to grok.

I'd wager maybe 70% of patches were able to apply with zero intervention on my end (the ones that show they're committed directly by the original authors were straight patches; the ones that begin with "From" are ones I had to do by hand, so those are the ones with the highest potential rate of error, while the rest are equivalent to having pulled it straight from the upstream repo.

@apanteleev
Copy link
Collaborator

I briefly looked at each of these 350 commits... Generally these are engine/networking/GL changes, seem relatively safe assuming that they've been around in Q2PRO for a while. We still need to test the updated Q2RTX pretty heavily though.

A few notes:

e95c3cb - Disable system DPI scaling on Windows.

Similar change is already there in Q2RTX, should probably remove the old files.

86c0931 - Make light styles always compiled in.

Need to make sure it doesn't break light styles in Q2RTX.

7db5656 - Parse userinfo ‘fov’ value as integer.

Why?

26b3007 - properly detect git repository, works in submodule too
c033650 - Simplify revision number calculation.

I don't think version.sh is used in q2rtx

8f15ac5 - Switch to the new QGL implementation.

Maybe we can remove the GL headers from the repo?

6c92593 - Add experimental GLSL rendering backend.

Nice, this addresses #153
Also, GL headers.

e575a98 - Using separate type for error values only adds clutter. Replace with int.

I disagree with the subject statement. Returning qerror_t declares that it's a status code and not some other int.

cf3a9f9 - Use SSE math by default for x86 builds.

These cmake changes should be conditional on x86/x64 target arch (not ARM).

2b60003 - Convert nextthink to frame numbers.
and a few more after it

Is this a good thing?...

23ef7b7 - Add previous weapon to key bindings menu.

q2pro.menu should be deleted

@res2k
Copy link
Contributor

res2k commented Dec 10, 2021

2b60003 - Convert nextthink to frame numbers. and a few more after it

Is this a good thing?...

Using integer frame numbers should be more robust for long-running games, when you get into float value ranges with insufficient precision to represent a difference of 0.1. At least that's my guess to the reason behind this change.

Of course this assumes that the frame rate visible by the game is always 10 fps. Looking at SV_Frame(), this is the case.

@res2k
Copy link
Contributor

res2k commented Dec 10, 2021

86c0931 - Make light styles always compiled in.

Need to make sure it doesn't break light styles in Q2RTX.

I looked at the commit. Unfortunately I noticed a change unrelated to lightstyles itself:

-   con_scale = Cvar_Get("con_scale", "1", 0);
+   con_scale = Cvar_Get("con_scale", "0", 0);

I noticed this by chance. Maybe it's the only instance of an unrelated change that slipped in.
Still, it isn't really that great if you can't rely on a commit doing what it's description says...

@Paril
Copy link
Contributor Author

Paril commented Dec 11, 2021

@res2k the 86c0931 commit issue was my fault, not Skuller. See skullernet/q2pro@dfdbce6

This change actually comes from skullernet/q2pro@e5a22c4
(commit 5870f6d in Q2RTX) but for whatever reason I accidentally shifted it into the next commit.

Like I said before, the ones that start with "From" are the ones that need additional scrutiny since they were ones I had to do by hand, so they are the most likely to contain issues like this. I appreciate the find! I'm not sure if I want to risk re-writing the Git history to fix this one though.

Of course this assumes that the frame rate visible by the game is always 10 fps. Looking at SV_Frame(), this is the case.

It is. These changes were probably just Skuller wanting to fix up what he saw as a legacy problem. unless sv_fps is supplied, in which case the server runs at a higher framerate, but he hasn't added support for that himself, he relied on other modders to add it to their own codebases.

e575a98 - Using separate type for error values only adds clutter. Replace with int.
I disagree with the subject statement. Returning qerror_t declares that it's a status code and not some other int.

He converted a lot of stuff to work similar to some C APIs that assume positive and 0 are good values, and negative ones are errors (for instance the file loading stuff). The change makes sense for things like that, because you don't want to assume that every return value is an error when it may not be, but I agree that this kind of semantic is hard to represent in C. Unfortunately it is a change that future commits rely on, but if you really dislike it we can adjust it.

Maybe we can remove the GL headers from the repo?

This is for Windows only. Windows requires them since Microsoft only provides GL 1.0 headers, and Q2PRO basically just ditched all of the custom qgl code and now relies on the system's glext. I did it for simplicity. It's up to you. Move it into VC maybe?

86c0931 - Make light styles always compiled in.
Need to make sure it doesn't break light styles in Q2RTX.

The commit just removes the USE_LIGHTSTYLES macro (and the one other side effect, as noted by res). It didn't touch any actual logic. Q2RTX forced it to 1, so this should be a no-op change.

7db5656 - Parse userinfo ‘fov’ value as integer.
Why?

FOV is sent in playerstate encoded as a byte, so this change is effectively just a data type change to match the server->client behavior and won't affect anything in practice.

@Paril
Copy link
Contributor Author

Paril commented Dec 11, 2021

Oh, I should also mention - I left all of the documentational changes out of these commits. I can do a final pass over the q2pro docs and change the q2rtx docs appropriately.

@Paril
Copy link
Contributor Author

Paril commented Dec 11, 2021

23ef7b7 - Add previous weapon to key bindings menu.
q2pro.menu should be deleted

Agreed, but having it be modified helped me be aware of any changes that were required; for instance choice menus use $$ instead of $ now. I did bring that over to q2rtx.menu.

@apanteleev apanteleev merged commit a879e7b into NVIDIA:master Dec 18, 2021
apanteleev added a commit that referenced this pull request Dec 18, 2021
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.

6 participants