Skip to content

Fix multiple regressions from mathlib rewrite #703

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix multiple regressions from mathlib rewrite #703

wants to merge 5 commits into from

Conversation

jengelh
Copy link
Contributor

@jengelh jengelh commented May 19, 2025 •
edited
Loading

Pull Request Type

  • Build and Dependency changes

Description

  1. Skip the unnecessary copy, especially when larger vectors/matrices are used.
  2. Add const qualifiers to parameters which are not used as output parameters.

Checklist

  • I have tested my changes locally and verified that they work as intended.
  • I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
  • I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

@Lgt2x
Copy link
Member

Lgt2x commented May 19, 2025

@jopadan this improves on #686 , can you please confirm that it is correct? Could you also comment on #686 (comment) ? I also think that the formula is applicable to any vector dimension.

@jengelh jengelh mentioned this pull request May 20, 2025
Open
5 tasks
jengelh added 3 commits May 21, 2025 16:35
@jengelh
lib: fix crash due to bogus type reinterpretation
c61acd1 
==78918==ERROR: AddressSanitizer: unknown-crash on address 0x7b8c9e36257c at pc
0x000001bdb39f bp 0x7ffdccbd8410 sp 0x7ffdccbd8408
READ of size 32 at 0x7b8c9e36257c thread T0
    f0  vm_Dot3Vector ~/lib/vecmat_external.h:421
    f1  operator*(matrix, matrix) ~/vecmat/vector.cpp:295
    f2  ReadNewModelFile ~/model/polymodel.cpp:1938
    f3  PageInPolymodel(int, int, float*) ~/model/polymodel.cpp:2189
    f4  GetPolymodelPointer(int) ~/model/polymodel.cpp:2214
    f5  ObjSetRenderPolyobj(object*, int, int) ~/Descent3/ObjInit.cpp:649
    f6  ObjInitGeneric(object*, bool) ~/Descent3/ObjInit.cpp:787
    f7  ObjInitTypeSpecific(object*, bool) ~/Descent3/ObjInit.cpp:1110
    f8  ObjInit(object*, int, int, int, vec<float, 3ul, (align)8, 8ul>*, float, int) ~/Descent3/ObjInit.cpp:1199
    f9  ReadObject(CFILE*, object*, int, int) ~/Descent3/LoadLevel.cpp:1657
    f10 LoadLevel(char*, void (*)(char const*, int, int)) ~/Descent3/LoadLevel.cpp:3884
    f11 LoadMissionLevel(int) ~/Descent3/Mission.cpp:1258
    f12 LoadAndStartCurrentLevel() ~/Descent3/gamesequence.cpp:1629
    f13 GameSequencer() ~/Descent3/gamesequence.cpp:1188
    f14 PlayGame() ~/Descent3/game.cpp:821
    f15 MainLoop() ~/Descent3/descent.cpp:552
    f16 Descent3() ~/Descent3/descent.cpp:505
    f17 oeD3LnxApp::run() ~/Descent3/sdlmain.cpp:143
    f18 main ~/Descent3/sdlmain.cpp:315
    f19 __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    f20 __libc_start_main_impl ../csu/libc-start.c:360
    f21 _start ../sysdeps/x86_64/start.S:115

Address 0x7b8c9e36257c is located in stack of thread T0 at offset 124 in frame
    #0 operator*(matrix, matrix) ~/vecmat/vector.cpp:290

  This frame has 2 object(s):
    [32, 68) 'src0' (line 290)
    [112, 148) 'src1' (line 290) <== Memory access at offset 124 partially overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: unknown-crash ~/lib/vecmat_external.h:421 in vm_Dot3Vector

Fixes: adda1c1
@jengelh
lib: pass vecmat parameters by reference
f93e01c 
Skip the unnecessary copy, especially when larger vectors/matrices
are used.
@jengelh
lib: add const qualifiers to vecmat function parameters
9a6b5da
@jengelh jengelh changed the title pass vector/matrices by (const) ref/pointer Fix crash from reinterpret_cast & pass vector/matrices by (const) ref/pointer for efficiency May 23, 2025
jengelh added 2 commits May 26, 2025 23:41
@jengelh
lib: fix refactoring mistake in vm_MakeInverseMatrix
eedd6aa 
Fixes: adda1c1
@jengelh
renderer: fix refactoring mistake in g3_StartFrame
6374ad3 
Revert a complete hunk from adda1c1.
Note how the z component was _not_ multiplied before adda1c1.

This fixes the missile camera being displayed in the wrong on-screen
location.

Fixes: adda1c1
@jengelh jengelh changed the title Fix crash from reinterpret_cast & pass vector/matrices by (const) ref/pointer for efficiency Fix multiple regressions from mathlib rewrite May 26, 2025
@jengelh jengelh mentioned this pull request May 26, 2025
Open
3 tasks
Lgt2x
Lgt2x approved these changes May 27, 2025
View reviewed changes
Copy link
Member

@Lgt2x Lgt2x left a comment

Choose a reason for hiding this comment

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

ugh I missed an awful lot of issues on the math lib rewrite, and nothing seemed wrong on my usual testing round... thanks for the effort on that. It could be nice to add some specific testing later to make sure it's rock-solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lgt2x Lgt2x Lgt2x approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jengelh @Lgt2x