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

Respect buffer's original dimension when updating reglBuffer data #669

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

Conversation

fuzhenn
Copy link
Contributor

@fuzhenn fuzhenn commented Sep 30, 2023

Currently, buffer's dimension will be always reset to 1 when calling buffer(array) to update its data.

This default behavior will lead to some incorrect context, e.g. wrong size parameter when calling gl.vertexAttribPointer in REGLVAO.

Respect buffer's original dimension will solve it.

@rreusser
Copy link
Member

rreusser commented Nov 12, 2024

Sorry this has sat for a long time, but it's my opinion that perhaps the current behavior is correct. The docs say:

To reinitialize a buffer in place, we can call the buffer as a function:

In that sense, I believe it's a full reinitialization with interpretation of arguments identical to the initial creation of the buffer, so that if you had to specify a dimension during initial creation, you should have to specify it during reinitialization as well.

Assuming some existing buffer, you can achieve the correct dimension by either specifying the dimension explicitly, e.g.

existingBuffer({
  data: [1, 2, 3],
  dimension: 3
})

or you can nest the data as an array of arrays, e.g.

existingBuffer([[1, 2, 3]])

If you want to replace data in-place without reinitializing, buffer.subdata is more performant and will preserve the dimension (though it requires the size is unchanged).

If this is still of interest, let me know if you have opinions. Thanks!

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.

2 participants