asafbibas / jmonkeyengine

Automatically exported from code.google.com/p/jmonkeyengine
0 stars 0 forks source link

Mesh.updateCounts() reads beyond buffer limit #503

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
What did you intend to do, what was the expected outcome?

i intended to have a big buffer of size 3000, this will be allocated only once 
and its capacity will never change again. Then depending on the number of 
sprites on scene, i will adapt the limit, so that only <limit> sprites are 
drawn at screen.

What outcome do you experience instead?

Jme draws <capacity> sprites on screen, regardless of limit.
The sprites [0, limit] are drawn successfully as expected.
The sprites [limit+1, capacity] are drawn (they shouldn't be), with undefined 
values which is illegal according to java buffer api that clearly states that 
java reads only from [0, limit].

Why this bug happens :

Jme's mesh.updateCounts() wrongly calculates the vert count, it calculates that 
vertCount should be 3000 (= buffer capacity) instead of 300 sprites ( = limit 
), thus makes jme read after buffer limit and create an extra 2700 sprites that 
are undefined (garbage sprites).

Paranoid considerations (Will patch affect code?):
1) The comments (javadoc) of the method should be updated, stating that it 
should also be used if limit is also changed.

2) Could tralala be crazy and picked the wrong method to modify ?
E.g i wanted my method to do apples and he comes and makes it do oranges.

 The original author may have intented this method to solve other issues and just picked the name update Counts at random out of coincidence (you never know, he may be drunk). I dont know about you but if something has as a name UpdateX(), it should update X for all cases. In this case we clearly see that X is wrongly updated if limit is smaller than capacity.

Furthermore updateCounts() is the only method in jme that modifies vertCount 
variable (the one that causes the problem). Thus it is this methods 
responsibility to fix it.

Changing the vertCount to be calculated as :
vertCount = pb.getData().limit() / pb.getNumComponents();
solves the problem of the testcase.

3) What about element count, should be calculated based on limit or based on 
capacity?

The testcase works even if elementCount is calculated using the .capacity(). 
However as we all know code is always symmetrical, if you change the way you 
calculate something, you should change it accordingly at all places. So i 
believe both should be calculated also based on limit, because else it would 
create an inconcistenses.

What steps will reproduce the problem?

see code and screenshots at 
http://jmonkeyengine.org/groups/contribution-depot-jme3/forum/topic/patch-in-mes
h-updatecounts/

patch:

public void updateCounts(){
        if (getBuffer(Type.InterleavedData) != null)
            throw new IllegalStateException("Should update counts before interleave");

        VertexBuffer pb = getBuffer(Type.Position);
        VertexBuffer ib = getBuffer(Type.Index);
        if (pb != null){
            vertCount = pb.getData().limit() / pb.getNumComponents();
        }
        if (ib != null){
            elementCount = computeNumElements(ib.getData().limit());
        }else{
            elementCount = computeNumElements(vertCount);
        }
    }

Any questions ?

Original issue reported on code.google.com by tralalol...@gmail.com on 4 Jun 2012 at 2:35

GoogleCodeExporter commented 8 years ago
Issue 502 has been merged into this issue.

Original comment by ShadowIs...@gmail.com on 4 Jun 2012 at 2:59

GoogleCodeExporter commented 8 years ago

Original comment by ShadowIs...@gmail.com on 4 Jun 2012 at 3:24