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

PyGLM types do not support PyBUF_SIMPLE #61

Closed
einarf opened this issue Jan 31, 2020 · 8 comments · Fixed by #119
Closed

PyGLM types do not support PyBUF_SIMPLE #61

einarf opened this issue Jan 31, 2020 · 8 comments · Fixed by #119
Assignees

Comments

@einarf
Copy link

einarf commented Jan 31, 2020

Example from mat_getbuffer

if ((flags & PyBUF_RECORDS_RO) != PyBUF_RECORDS_RO || (flags & PyBUF_F_CONTIGUOUS) == PyBUF_F_CONTIGUOUS || (flags & PyBUF_ANY_CONTIGUOUS) == PyBUF_ANY_CONTIGUOUS) {
	PyErr_SetString(PyExc_BufferError, "This type of buffer is not supported.");
	view->obj = NULL;
	return -1;
}

Most types (if not all) do pass the PyBuffer_IsContiguous() tests, so I assume all types have contiguous buffer data. Still, the various *_getbuffer methods do not accept this flag.

We discovered this issue when trying to set uniform or buffer data in moderngl.

program['uniform'].value = (1, 1, 1)    # working
program['uniform'].write(np.array([1,1,1]))    # working

v = glm.fvec3(1,1,1)
program['uniform'].write(memoryview(v))    # working
program['uniform'].write(bytes(v))    # working
program['uniform'].write(np.array(v))    # working

program['uniform'].write(v)    #  <--------- error

Error output:

BufferError: This type of buffer is not supported.

We obtain the buffer using:

PyObject_GetBuffer(value, &buffer_view, PyBUF_SIMPLE);

This is acceptable when you just want to write contiguous to an opengl buffer and have no plans to keep the buffer reference or modify it.

More details here:
moderngl/moderngl#379

@Zuzu-Typ
Copy link
Owner

I'm not sure I understand what you're getting at.

Most types (if not all) do pass the PyBuffer_IsContiguous() tests, so I assume all types have contiguous buffer data. Still, the various *_getbuffer methods do not accept this flag.

PyBUF_SIMPLE has nothing to do with contiguity. It's about the structure of the data (see here).

It doesn't make sense to me for PyGLM to provide buffers with less complexity than PyBUF_STRIDES.
The buffers it provides needs to provide information about the shape (e.g. (4, 4) for mat4x4) and stride (e.g. sizeof(double) for dmat4x4). Otherwise the buffer data would be useless (unless the calling function knew the shape anyway).

PyGLM accepts any buffer requests that have at least the PyBUF_RECORDS_RO flag and not fortan contiguity (because the data is layed out in C contiguity).

@Zuzu-Typ
Copy link
Owner

I guess I could add support for less complex buffer structures in case a program wants that.

@einarf
Copy link
Author

einarf commented Feb 1, 2020

Sorry, I was rambling a bit trying to figure things out while learning in the process 😄

Yes it would definitely be nice to have support for less complex buffer structures so they can be used just like memoryview, bytes and np arrays. Could of course just wrap them in a memoryview or something, but that does seem unnecessary.

Zuzu-Typ added a commit that referenced this issue Feb 2, 2020
Moved PyGLM's code to it's own files.
Also fixed #60
And fixed #61
@Zuzu-Typ Zuzu-Typ self-assigned this Feb 2, 2020
@einarf
Copy link
Author

einarf commented Feb 2, 2020

Works now. Thank you.

@aforren1
Copy link
Contributor

It looks like a6c0f1f introduced a strides check, so PyBUF_SIMPLE requests don't seem to work from v2.0.0a3 onward. Any chance that check can be removed so PyBUF_SIMPLE works again, or am I missing some other detail? Thanks!

@Zuzu-Typ
Copy link
Owner

It looks like a6c0f1f introduced a strides check, so PyBUF_SIMPLE requests don't seem to work from v2.0.0a3 onward. Any chance that check can be removed so PyBUF_SIMPLE works again, or am I missing some other detail? Thanks!

I don't know why exactly I added that check back in.
I'll fix it right away.
Thanks for letting me know (:

@Zuzu-Typ Zuzu-Typ reopened this Apr 12, 2021
Zuzu-Typ added a commit that referenced this issue Apr 12, 2021
Removed checks for strides in mat_getbuffer and checks for non-fortran-contiguity in the remaining getbuffer methods.

Should fix #61
Zuzu-Typ added a commit that referenced this issue Apr 12, 2021
Removed checks for strides in mat_getbuffer and checks for non-fortran-contiguity in the remaining getbuffer methods.

Should fix #61
@aforren1
Copy link
Contributor

Great, thanks for the fix!

@einarf
Copy link
Author

einarf commented Apr 12, 2021

Thanks! I just noticed this late last night when upgrading pyglm. Writing uniform data to shaders did not work any more in moderngl and arcade using the buffer protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants