[aquaria] [PATCH] Fix logic bugs in reordering of RenderObjects within a layer.
False.Genesis
false.genesis at googlemail.com
Sat Sep 3 00:11:05 EDT 2011
Reproduced it by starting up at the final save point, and then staying
idle in sun form for ~20 secs. Crash.
The problem is that these 2 calls ...
lightFormGlow->moveToFront();
lightFormGlowCone->moveToFront();
...do a move front all the time, alternating, leaving a lot of NULL
vector entries behind, until "firstFreeIdx = curIdx" reaches the end of
the vector (preallocated with 100), and overflows the index in the while
loop pointed out earlier.
As this happens only every 0.5s, it takes a while to actually reach the end.
The following snippet fixes it, but i suppose it is too hackish.
Is the vector ever compacted? Seems to me that RenderObjectLayer::sort()
is never actually called, and it does not set firstFreeIdx accordingly
anyway - it should, imho. If it was called.
diff -r 613cf0ce4519 BBGE/RenderObjectLayer.cpp
--- a/BBGE/RenderObjectLayer.cpp Thu Aug 18 20:44:43 2011 +0900
+++ b/BBGE/RenderObjectLayer.cpp Sat Sep 03 05:54:26 2011 +0200
@@ -263,7 +263,11 @@
r->setIdx(lastUsed);
firstFreeIdx = curIdx;
while (renderObjects[firstFreeIdx])
+ {
firstFreeIdx++;
+ if(firstFreeIdx >= size)
+ firstFreeIdx = 0;
+ }
}
#endif // RLT_FIXED
#ifdef RLT_DYNAMIC
Am 03.09.2011 04:33, schrieb Andrew Church:
> Can you find a way to consistently reproduce it? I can't do so here.
> I'm happy to look into it if so, but otherwise your best bet may be to
> switch back to RLT_DYNAMIC and linked list management -- RLT_FIXED was
> mainly to avoid cache demolition on the PSP from all the pointer lookups,
> and RLT_DYNAMIC shouldn't be significantly slower on PCs.
>
> FWIW, I have a more robust solution for the iPad port, but unfortunately
> it's nontrivial to backport (the iPad port completely replaces BBGE with
> a new engine).
>
> --Andrew Church
> achurch at achurch.org
> http://achurch.org/
>
>> This patch introduced a crash bug with the sun form (found by Sindhi).
>> It goes out of index in a while (renderObjects[firstFreeIdx]) loop, when
>> called from void Avatar::updateFormVisualEffects(), which uses
>> moveToFront() twice. Seems it expects a NULL in the vector although
>> there is none.
>> Andrew, you wrote the code, any clue what is going wrong?
>>
>> -- fg
>>
>>
>>
>> Am 17.08.2011 13:54, schrieb Andrew Church:
>>> # HG changeset patch
>>> # User Andrew Church<achurch at achurch.org>
>>> # Date 1313581717 -32400
>>> # Node ID 9c350f140a62de19546039b36d88fd8e0e7d3442
>>> # Parent 5542b94cae02a6333845854bbbd1abe0a259f1a4
>>> Fix logic bugs in reordering of RenderObjects within a layer.
>>>
>>> This patch fixes logic bugs with state management for the array-based
>>> RenderObjectLayer implementation which could cause crashes when an
>>> object was moved to the back of the layer.
>>>
>>> This is the correct fix for the bug worked around in r199 (b00e51cba5bc).
>>>
>>> diff -r 5542b94cae02 -r 9c350f140a62 BBGE/RenderObjectLayer.cpp
>>> --- a/BBGE/RenderObjectLayer.cpp Mon Aug 01 14:16:11 2011 -0400
>>> +++ b/BBGE/RenderObjectLayer.cpp Wed Aug 17 20:48:37 2011 +0900
>>> @@ -229,8 +229,8 @@
>>> renderObjects[curIdx] = 0;
>>> renderObjects[newIdx] = r;
>>> r->setIdx(newIdx);
>>> - if (firstFreeIdx == newIdx)
>>> - firstFreeIdx++;
>>> + if (firstFreeIdx> curIdx)
>>> + firstFreeIdx = curIdx;
>>> }
>>> else if (objectCount == size)
>>> {
>>> @@ -242,7 +242,7 @@
>>> r->setIdx(size);
>>> for (int i = size+1; i< newSize; i++)
>>> renderObjects[i] = 0;
>>> - firstFreeIdx = size+1;
>>> + firstFreeIdx = curIdx;
>>> }
>>> else
>>> {
>>> @@ -254,18 +254,16 @@
>>> if (!renderObjects[lastFree])
>>> break;
>>> }
>>> -
>>> for (int i = lastFree + 1; i<= lastUsed; i++)
>>> {
>>> renderObjects[i-1] = renderObjects[i];
>>> - if(renderObjects[i-1])
>>> - renderObjects[i-1]->setIdx(i-1);
>>> + renderObjects[i-1]->setIdx(i-1); // Known to be non-NULL.
>>> }
>>> -
>>> renderObjects[lastUsed] = r;
>>> r->setIdx(lastUsed);
>>> - if (firstFreeIdx == lastFree)
>>> - firstFreeIdx = lastUsed + 1;
>>> + firstFreeIdx = curIdx;
>>> + while (renderObjects[firstFreeIdx])
>>> + firstFreeIdx++;
>>> }
>>> #endif // RLT_FIXED
>>> #ifdef RLT_DYNAMIC
>>> @@ -298,7 +296,9 @@
>>> renderObjects[curIdx] = 0;
>>> renderObjects[newIdx] = r;
>>> r->setIdx(newIdx);
>>> - if (firstFreeIdx == newIdx)
>>> + // firstFreeIdx must be 0 here; if we filled slot 0, then
>>> + // scan forward for the next empty element.
>>> + while (renderObjects[firstFreeIdx])
>>> firstFreeIdx++;
>>> }
>>> else if (objectCount == size)
>>> @@ -312,8 +312,7 @@
>>> for (int i = newSize - 1; i>= sizeDiff; i--)
>>> {
>>> renderObjects[i] = renderObjects[i - sizeDiff];
>>> - if(renderObjects[i])
>>> - renderObjects[i]->setIdx(i);
>>> + renderObjects[i]->setIdx(i); // Known to be non-NULL.
>>> }
>>> for (int i = 0; i< newIdx; i++)
>>> renderObjects[i] = 0;
>>> @@ -329,8 +328,7 @@
>>> for (int i = firstFreeIdx; i> 0; i--)
>>> {
>>> renderObjects[i] = renderObjects[i-1];
>>> - if(renderObjects[i])
>>> - renderObjects[i]->setIdx(i);
>>> + renderObjects[i]->setIdx(i); // Known to be non-NULL.
>>> }
>>> renderObjects[0] = r;
>>> r->setIdx(0);
>>> _______________________________________________
>>> aquaria mailing list
>>> aquaria at icculus.org
>>> http://icculus.org/mailman/listinfo/aquaria
>>>
>> _______________________________________________
>> aquaria mailing list
>> aquaria at icculus.org
>> http://icculus.org/mailman/listinfo/aquaria
> _______________________________________________
> aquaria mailing list
> aquaria at icculus.org
> http://icculus.org/mailman/listinfo/aquaria
>
More information about the aquaria
mailing list