ofVbo: don't use stride to calculate the buffer length #2829

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

ofVbo: don't use stride to calculate the buffer length

In current implementation of setAttributeData/updateAttributeData the stride argument is used for two orthogonal concepts:

  1. to calculate the size of the buffer
  2. as an argument of glVertexAttribPointer

this make impossible to use a tightly packed array as an argument to a shader.

@bilderbuchi bilderbuchi commented on the diff Mar 5, 2014

libs/openFrameworks/gl/ofVbo.cpp
@@ -426,7 +426,7 @@ void ofVbo::setAttributeData(int location, const float * attrib0x, int numCoords
attributeNumCoords[location] = numCoords;
@bilderbuchi

bilderbuchi Mar 5, 2014

Owner

as you have changed the default argument of stride to 0, this can lead to a different value for attributeStrides[location]. have you considered the impact of this?

Owner

bilderbuchi commented Mar 5, 2014

@openframeworks/2d-3d could you take a look at this?

Owner

arturoc commented Mar 5, 2014

i think there's still one case missing, when you have a different stride than the total size of the buffer this won't work, it should be something like:

if(stride==0){
    dataSize = numelements*numcoords*sizeof(float);
}else{
    dataSize = stride*numelements;
}

arturoc, I miss your point, why the stride argument must be used to calculate the buffer size? it should be used only with glVertexAttribPointer isn't it?

regarding the signature change, I submit this PR mainly to discuss the problem; due to the semantic change the right thing to do (merge, introduce new methode or whatever) is a decision only you can make.

Member

tgfrerer commented Mar 5, 2014

@cinghiale great catch! i think the most important change is to make sure OpenGL loads in the correct ammount of data.

It might make sense to store the dataSize (as we store attributeStrides) as a member variable of ofVbo.

If we store dataSize[location], we could use this in the updateAttributeData method.

@tgfrerer You want to store the dataSize in order to add an additional check to updateSetAttribute so we can be sure to not pass a total size greater than the original one?

If IIUC the docs it's perfectly fine to call glBufferSubData on a subset of the original buffer

Member

tgfrerer commented Mar 5, 2014

i thought about the additional check, too, but since we are receiving only a float* and not e.g. a vector<float>& there's nothing to check against, and i think such an additional check would be over-engineering at this point.

If we did check, it would be a greater set, rather than a subset to worry about, that's right.

Owner

arturoc commented Mar 5, 2014

@cinghiale if you have something like:

struct vertex{
    ofVec3f attribute;
    float padding; // to round to 16bits, makes it faster to upload
}

or you are passing a vector of a class with other variables you don't want to use then numelements_numcoords_sizeof(float) won't match, you need to pass numelements*stride

@arturoc you are right, maybe we can use: numelements_(numcoords_sizeof(float)+stride)

Member

tgfrerer commented Mar 5, 2014

Tricky! You can't just add stride, since stride is not strictly an offset about padding, but holds the distance in bytes between consecutive elements (unless you specify 0 for tightly packed):

from OpenGL Programming Guide, 8th ed., p.26:

"stride is the byte offset between consecutive elements in the array. If stride is zero, the data is assumed to be tightly packed."

i think @arturoc's dataSize approach is pretty solid =)

Owner

arturoc commented Mar 5, 2014

yes you can't add the stride like that, in the example i posted before:

struct vertex{
    ofVec3f attribute;
    float padding; // to round to 16bits, makes it faster to upload
}

the stride is sizeof(vertex)==16, numcoords*sizeof(float)+stride will give you 28

i've used the method i posted before several times and it works for all cases:

if(stride==0){
    dataSize = numelements*numcoords*sizeof(float);
}else{
    dataSize = stride*numelements;
}

@arturoc @tgfrerer you are right, I have misunderstood the reference.

with a struct like this

struct vertex{
    ofVec3f attribute;
    float padding; // to round to 16bits, makes it faster to upload
}

the stride value should be sizeof(vertex) (== 4 * sizeof(float)); if this is correct maybe the default value for the stride argument is misleading?

Owner

arturoc commented Mar 5, 2014

0 will work for 99% of the cases and the stride is a concept difficult to grasp so i think it's better to default it to 0. people who want to do more complex stuff will know that they need to pass a explicit stride

Member

tgfrerer commented Mar 10, 2014

Hey guys! Because i was afraid this might get stale & i needed a bugfix pronto =), i'm sending a PR that should address the bug & the things we discussed...

arturoc closed this in 11f61a8 Mar 10, 2014

@arturoc arturoc added a commit that referenced this pull request Mar 10, 2014

@arturoc arturoc Merge pull request #2839 from tgfrerer/fix-ofVboStride
fixes #2829 ofVbo custom attribute stride
5bb876c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment