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

Fix pre-rendered cutscene corruption #188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dje4321
Copy link

@dje4321 dje4321 commented May 22, 2024

Fixes movie corruption due to a bad return type in getOffset().

Dword_51F018 is a 256 element arry with 16 bit values ranging from 65535 - -65535. Elements 0-127 are all positive 16bit values where as elements 128-255 are all negative 16bit values. getOffset() takes in an unsigned 16bit value, gets the signed representation of the lower 8 bits and adds it to the result of the Dword_51F018[] array after indexing the upper 8 bits of the value

Return type is capped at 16bits incase of an intentional overflow so the offset calculation does not exceed dest ptr of the memcpy() call during the decompression stage of the video.

Not 100% on this fix as I have not fully reversed the decompression function to ensure the behavior of getOffset() and the decompression function are proper in tune with each other. However I have verified that this fixes both the intro cinematic and the vat explosion cutscene near the end of the game.

Credit goes to @Northfear for their vita fork for pointing me in the right direction on where to start with this issue. See commit 00c1269 on their fallout1-ce-vita fork for their fix. Due to the inconsistancy of the C STD library, I performed further verification of the fix because the bit width of int has no guarntees. This can lead to platform->platform issues as each compiler and triple target could assign int to be whatever bit width most suits it.

Dword_51F018 is a 256 element arry with 16 bit values ranging from
65535 - -65535. Elements 0-127 are all positive 16bit values where
as elements 128-255 are all negative 16bit values. getOffset() takes
in an unsigned 16bit value, gets the signed representation of the
lower 8 bits and adds it to the result of the Dword_51F018[] array
after indexing

Return type is capped at 16bits incase of an intentional overflow
so the offset calculation does not exceed dest ptr of the memcpy()
call during the decompression stage of the video.

Not 100% on this fix as I have not fully reversed the decompression
function to ensure the behavior of getOffset() and the decompression
function are proper in tune with each other. However I have verified
that this fixes both the intro cinematic and the vat explosion
cutscene near the end of the game.

Credit goes to @Northfear for their vita fork for pointing me in the
right direction on where to start with this issue. See commit
00c1269 on their fallout1-ce-vita
fork for their fix. Due to the inconsistancy of the C STD library,
I performed further verification of the fix because the bit width
of int has no guarntees. This can lead to platform->platform issues
as each compiler and triple target could assign int to be whatever
bit width most suits it.
@dje4321
Copy link
Author

dje4321 commented May 22, 2024

Attached is a graph of the Dword_51F018 array mapping that gets generated at runtime
graph

@dje4321
Copy link
Author

dje4321 commented May 22, 2024

One issue remains that the border surrounding the FMV cutscene is still remains corrupted at times. However, it appears to be an issue with SDL not refreshing the border surrounding the FMV with a new blank background

@@ -2802,7 +2802,7 @@ constexpr uint32_t loadUInt32LE(const uint8_t* b)
return (b[3] << 24) | (b[2] << 16) | (b[1] << 8) | b[0];
}

uint8_t getOffset(uint16_t v)
int16_t getOffset(uint16_t v)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be int (or int32_t).
In my original code it was effectively int (the type was changed during a refactoring at merge time, see #117 (comment))

Per you graph, some values won't fit in int16_t.

Copy link
Contributor

Choose a reason for hiding this comment

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

(while int can technically be as small as 16-bit, fallout won't run on such platforms anyway)

@giuliano-macedo
Copy link

Fixes #194 even if using int16_t, int32_t or int64_t as a return type for getOffset

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.

None yet

3 participants