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

Get all changes from latest dhewm3-sdk that are missing #67

Merged
merged 33 commits into from
Jan 8, 2025

Conversation

DanielGibson
Copy link
Contributor

All commits that were made after January 2019, except ones affecting the README or CMakeLists.txt.
All the CMakeLists.txt changes are combined in the last commit ("Update CMakeLists.txt to current dhewm3-sdk state").

I added the dhewm3-sdk repo as a git remote and cherry-picked the commits, like

git remote add sdk [email protected]:dhewm/dhewm3-sdk.git
git fetch sdk # fetch all the changes from the dhewm3-sdk repo into the local cache
# the following line cherry-picks the commits from
# from 74cacadd "Increase Script MAX_GLOBALS in game/"
# to   1e80eac3 "Fix "t->c->value.argSize == func->parmTotal" Assertion in Scripts, #303"
git cherry-pick 74cacadd738d63bb4ea89313ee59d4e670df7c94..1e80eac345b4bb6f71655239f50ec2585f67b64b
# I left the next commit out because it only affected CMakeLists.txt
# and then did further git cherry-pick commands for the next commits etc

Sometimes I had to resolve merge conflicts, commit the resolved changes and then run git cherry-pick --continue so the remaining commits of the cherry-pick-range got applied.

DanielGibson and others added 30 commits January 8, 2025 02:56
On Windows, ID_INLINE does __forceinline, which is bad if the function
calls alloca() and is called in a loop..
So use just __inline there so the compiler can choose not to inline
(if called in a loop).
This didn't cause actual stack overflows as far as I know, but it could
(and MSVC warns about it).

(This includes "Fix ID_MAYBE_INLINE on non-Windows platforms")
idStr::StripFileExtension() (and SetFileExtension() which uses it) and
others didn't work correctly if there was a dot in a directory name,
because they just searched from last to first char for '.', so if the
current filename didn't have an extension to cut off, they'd just cut
off at any other '.' they found.
So D:\dev\doom3.data\base\maps\bla could turn into D:\dev\doom3
(or, for SetFileExtension(), D:\dev\doom3.map)

While at it, I made most of the idStr code that explicitly checked for
'\\' and '/' (and maybe ':' for AROS) use a little
"bool isDirSeparator(int c)" function so we don't have the #ifdefs
for different platforms all over the place.
If you save, you get a message like "Game Saved..." which goes away
after a few seconds. This happens at the very end of idPlayer::Save():
    if ( hud ) {
        hud->SetStateString( "message", /* .. left out .. */ );
        hud->HandleNamedEvent( "Message" );
    }
And handled in hud.gui, "onNamedEvent Message { ..."

However, if you save again before it's gone, it'll be shown after
loading the savegame and not go away until you save again..
This works around that issue by setting an empty message after loading
a savegame.

The underlying problem (which is not fixed!) seems to be that the
transition GUI command (that's executed when hud.gui handles the
"Message" event that's used to show this message) is probably not
properly saved/restored so fading out the message isn't continued
after loading.
It corrupted the stack when called with buffers allocated on the stack
and numSamples that are not a multiple of four, apparently, by writing
4 floats too many, at least in the 22KHz Stereo case..

This caused the crash described in
  dhewm/dhewm3#303 (comment)

Now it just uses the generic C code, like all platforms besides MSVC/x86
already do.
If a "class" (object) in a Script has multiple member function
prototypes, and one function implementation later calls another before
that is implemented, there was an assertion when the script was parsed
(at map start), because the size of function arguments at the call-site
didn't match what the function expected - because the function hadn't
calculated that size yet, that only happened once its own
implementation was parsed.
Now it's calculated (and stored) when the prototype/declaration is
parsed to prevent this issue, which seems to be kinda common with Mods,
esp. Script-only mods, as the release game DLLs had Assertions disabled.
like the dhewm3 version and the OS and architecture of the dhewm3
version that created the savegame.
Also added an internalSavegameVersion so be independent of BUILD_NUMBER

fixes #344
idClipModel::axis is an idMat3 rotation matrix.
Usually it's an identity matrix, but if the player is pushed around by
an idPush entity it's modified and apparently can (wrongly) remain
modified, possibly when saving while idPush is active.
This seems to happen sometimes on the crashing elevator in game/delta1.
The fix/workaround is to reset it to mat3_identity when loading a
savegame.
"Don't use memcpy in idMat2 constructor" up to "Don't use memset to zero
non-trivial type volumeIntegrals_t" from Turo Lamminen (in orig dhewm3
repo)
…that

All pointer<->integer conversion truncation warnings I'm aware of are
now enabled for MSVC, to hopefully finally get that tool code 64bit-clean.

I had to adjust the idCVar code for this - it should've been safe enough
(highly unlikely that a valid pointer is exactly 0xFFFFFFFF on 64bit),
but triggered those warnings - now it should behave the same as before
but the warnings (which are now errors) are silenced.
idStr is used in both the main thread and the async sound thread, so
it should better be thread-safe.. idDynamicBlockAlloc is not.
Use realloc() and free() instead.

For some reason this caused a lot more crashes (due to inconsistencies
in the allocator's heap) with newer Linux distros (like XUbuntu 20.04)
and when using GCC9, while they rarely reproduced with GCC7 or on
XUbuntu 18.04

fixes #391
These are now used by idStr::(v)snPrintf(), and in the future can
be used if a (v)snprintf() that's guaranteed not to call
common->Warning() or similar is needed (e.g. used during early startup)
only affect glconfig_t which I don't think any mod should use
…in tool code

only the TypeInfo changes are applicable to the SDK
to guarantee \0-termination
(only the applicable parts of that dhewm3 commit)
- TestHugeTranslation() prints positions (and asserts only afterwards),
  from "Work around assertion in alphalabs4, fix #409"
- idCompiler::CompileFile() prints proper filename
  from "Fix usage of invalid pointer in idCompiler::CompileFile()"
Some entities wrote the handle from gameRenderWorld->AddLightDef()
into savegames and reused it after restoring it.
That's a bad idea, because at that point the handle most likely belongs
to something else (likely some idLight). The most visible issue this
can create is that the flashlight may not work correctly after loading
a savegame with flashlight on, when it happens to alias a light that's
updated each frame to (mostly) being off..

The correct way to handle this (THAT FOR SOME REASON WAS ALREADY
IMPLEMENTED IN D3XP BUT NOT THE BASE GAME - WHY?!) is to get a fresh
handle with AddLightDef() when restoring a savegame - unless the handle
was -1, which means that the light didn't exist when saving.

fixes #495
.. and even removed in C++17 - and before that it probably didn't do
much in most compilers
shouldn't hurt too much, and the R_IssueEntityDefCallback() code
even is a bit better now
this was so broken, I think if anyone ever tried to use __DATE__ or
__TIME__ it must've crashed, either from free(curtime) (which was the
return value of ctime()) or from things like token[7] = NULL when token
was a pointer (to a single element!).

I think it could work now.

Also fixed potential memory leaks in cases that don't "return" anything
or, in the case of AF.cpp, at least comment on them

I think this fixes most of the remaining "interesting" GCC 12 on Linux
warnings
Workaround for MCST-LCC compiler < 1.28 version

mcst-lcc < 1.28 does not support __builtin_alloca_with_align()
Ref: http://mcst.ru/lcc
not sure if any of this is relevant for mods, but shouldn't hurt..
turns out that __builtin_alloca_with_align() might releases the
allocated memory at the end of the block it was allocated in, instead
of the end of the function (which is the behavior of regular alloca()
and __builtin_alloca()): "The lifetime of the allocated object ends at
 the end of the block in which the function was called. The allocated
 storage is released no later than just before the calling function
 returns to its caller, but may be released at the end of the block in
 which the function was called."
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005falloca_005fwith_005falign

Clang also supports __builtin_alloca_with_align(), but always releases
the memory at the end of the function.

And it seems that newer GCC versions also tend to release it at the
end of the function, but GCC 4.7.2 (that I use for the official Linux
release binaries) didn't, and that caused weird graphical glitches.
But as I don't want to rely on newer GCC versions behaving like this
(and the documentation explicitly says that it *may* be released at
 the end of the block, but will definitely be released at the end of
 the function), I removed all usage of __builtin_alloca_with_align().

(Side-Note: GCC only started documenting the behavior of
 __builtin_alloca and __builtin_alloca_with_align at all with GCC 6.1.0)
the latest dhewm3-sdk commit that affected CMakeLists.txt that is
integrated here is 94b42895270a03d0fa8841b10f171a0d6c80aee2
  "CMake: Detect all variations of the clang compiler (hopefully)"
@Stradex
Copy link
Owner

Stradex commented Jan 8, 2025

A million thanks!.

@Stradex Stradex merged commit 9d03fea into Stradex:master Jan 8, 2025
4 checks passed
@DanielGibson
Copy link
Contributor Author

uhh I just realized that a few commits got left out - looks like git cherry-pick f00..baa doesn't cherry-pick the first commit (f00) of the range -_-

I'll create a second PR with the missing commits.

Stradex added a commit that referenced this pull request Jan 11, 2025
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.

4 participants