-
-
Notifications
You must be signed in to change notification settings - Fork 149
Fix ZDE in PackedBitVector #334
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
base: master
Are you sure you want to change the base?
Conversation
UnityPy/helpers/PackedBitVector.py
Outdated
|
||
# avoid zero division | ||
scale_denominator = (1 << packed.m_BitSize) - 1 | ||
if scale_denominator == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original C++ code we have
uint32_t x;
float scale;
x &= (1 << m_BitSize) - 1;
data[i] = (x / (scale * ((1 << (m_BitSize)) - 1))) + m_Start;
Assuming that m_BitSize is 0, then x is 0 as well.
With this we get
data[i] = (0 / (scale * 0)) + m_Start;
Based on the IEEE 754 rules:
finite * 0.0 = 0.0 → 0 / 0.0 = NaN
±∞ * 0.0 = NaN → 0 / NaN = NaN
NaN * 0.0 = NaN → 0 / NaN = NaN
so with this result will always be NaN.
Which means that we can effectively return early with an array of NaN if m_BitSize is 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, there is just a little extra optimization I would like for this special case.
@K0lb3 Decompilation from 2022.3.21f1 development DLL reveals quite some changes from the Unity 4 reference code - TL;DR in such cases we probably should return an array of void __fastcall PackedFloatVector::UnpackFloats(
PackedFloatVector *this,
float *data,
int itemCountInChunk,
int chunkStride,
int start,
int numChunks)
{
__int64 itemCountInChunk_1; // r10
int m_BitSize; // ecx
float *data_1; // r15
double scale; // xmm2_8
int indexPos; // ebx
int bitPos; // r8d
unsigned int numChunks_1; // eax
__int64 itemCountInChunk_2; // r12
float *end; // r13
__int64 chunkStride_1; // rax
__int64 indexPos_1; // r9
__int64 i; // rsi
int m_BitSize_1; // r10d
int x; // edi
int bits; // r11d
int num; // ecx
__int64 indexPos_pp; // rax
int v24; // edx
int v25; // eax
__int64 chunkStride_2; // [rsp+30h] [rbp+8h]
itemCountInChunk_1 = itemCountInChunk;
m_BitSize = this->m_BitSize;
data_1 = data;
scale = this->m_Range;
indexPos = start * m_BitSize / 8;
bitPos = start * m_BitSize % 8;
// !! scale div by 0 skipped if m_BitSize is 0
// -> scale = m_Range
if ( m_BitSize )
scale = scale / ((1 << m_BitSize) - 1);
numChunks_1 = numChunks;
if ( numChunks == -1 )
numChunks_1 = this->m_NumItems / itemCountInChunk_1;
itemCountInChunk_2 = itemCountInChunk_1;
end = (data + (numChunks_1 * chunkStride));
if ( data != end )
{
chunkStride_1 = chunkStride;
chunkStride_2 = chunkStride;
indexPos_1 = indexPos;
do
{
i = 0i64;
if ( itemCountInChunk_2 > 0 )
{
do
{
m_BitSize_1 = this->m_BitSize;
x = 0;
bits = 0;
if ( this->m_BitSize )
{
do
{
x |= *(indexPos_1 + this->data) >> bitPos << bits;
num = m_BitSize_1 - bits;
if ( 8 - bitPos < m_BitSize_1 - bits )
num = 8 - bitPos;
indexPos_pp = indexPos_1 + 1;
bits += num;
v24 = num + bitPos; // std::min( m_BitSize-bits, 8-bitPos)
if ( num + bitPos != 8 )
indexPos_pp = indexPos_1; // indexPos++
indexPos_1 = indexPos_pp;
v25 = indexPos + 1;
if ( v24 != 8 )
v25 = indexPos;
bitPos = 0;
indexPos = v25;
if ( v24 != 8 )
bitPos = v24;
}
while ( bits < m_BitSize_1 );
} // !! Jump here if m_BitSize is 0
// -> data[i] = 0 * scale + this->m_Start
// -> data[i] = this->m_Start
data_1[i++] = (x & ((1 << m_BitSize_1) - 1)) * scale + this->m_Start;
}
while ( i < itemCountInChunk_2 );
chunkStride_1 = chunkStride_2;
}
data_1 = (data_1 + chunkStride_1);
}
while ( data_1 != end );
}
} It should also be noted that it's impossible to produce |
I agree with @mos9527 , we probably should return an array of |
@@ -22,7 +22,7 @@ def unpack_ints( | |||
start: int = 0, | |||
count: Optional[int] = None, | |||
shape: Optional[Tuple[int, ...]] = None, | |||
) -> List[int]: | |||
) -> List[Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I change it to List[Any]
is that the return type may also be List[List[int]] | List[List[List[int]]]
after the reshape
.
If you want, I can use @overload
to optimize the type annotation, which is the best practice but may be too verbose.
@overload
def unpack_ints(
packed: "PackedBitVector",
start: int = 0,
count: Optional[int] = None,
*,
shape: None = None,
) -> List[int]: ...
@overload
def unpack_ints(
packed: "PackedBitVector",
start: int = 0,
count: Optional[int] = None,
*,
shape: Tuple[int] = None,
) -> List[List[int]]: ...
@overload
def unpack_ints(
packed: "PackedBitVector",
start: int = 0,
count: Optional[int] = None,
*,
shape: Tuple[int, int] = None,
) -> List[List[List[int]]]: ...
Summary
Fixes #333
Test
Tested with the sample file provided in #333 .
(Left: AssetStudio, Right: Ours)