_moderngl.Error: wrong shader source type on str subclass about moderngl HOT 12 CLOSED

The bytes support is new, previously it caused a fault.

Can you provide an example when would you use a string subclass?

Whan I can think of as a fix is to add a third path converting anything else to string similar to python str(...).
Despite it may work I do not want to pass arbitrary types to PyUnicode_AsUTF8 or similar.

from moderngl.

Sure thing, here's my use case. One of the projects I'm working on makes use of internals of the garbage collector (dangerous, we know!), and it only works with objects that are tracked by the gc. Moreover, the objects are often sources of shaders, so we have defined

class ShaderSource(str):
    pass

This ensures that ShaderSource objects are tracked, which allows us to hook them into our system without requiring the use of a wrapper type where you have to access the string through a .data, .as_str() or similar.

PyUnicode_AsUTF8 checks its own arguments using PyUnicode_Check so I think it is "safe" to pass anything that passes the PyUnicode_Check check in the sense that it won't segfault, although it may cause unexpected behavior as it walks the internal representation of the unicode part of the object, ignoring any special functionality the user may have defined (similar to having a defaultdict raise a KeyError for a missing key instead of inserting it in a function that walks the dict memory directly).

With that said, an opt-in approach is also very reasonable and easy on mgl users, and it would allow supporting even more types such as user-defined wrappers of strings (e.g. ShaderSource if ShaderSource inherited from collections.UserStr instead). After thinking about it I think that this is probably the right way to go.

Here's a few names to get the brainstorm going for the conversion function:

• as_str
• as_shader_str
• shader_source (property)
• source_str (property)
• shader_str (property)
• get_source
• get_source_str
• get_shader_source
• get_shader_str
• to_str

Any favorites stick out to you?

from moderngl.

Thank you for the clear explanation.

PyUnicode_AsUTF8 checks its own arguments using PyUnicode_Check so I think it is "safe" to pass anything that passes the PyUnicode_Check check in the sense that it won't segfault.

Thank you for looking that up.
This is very interesting for me as it looks "thin ice". What I mean here, is that you can create arbitrary C structs with the Py_TPFLAGS_UNICODE_SUBCLASS flag set and you get passed the check.

I think it is ok to use PyUnicode_Check then.

With that said, an opt-in approach is also very reasonable and easy on mgl users,

for the fallback path I can think of to_shader_source(self) -> str.
Naming is similar to numpy's tolist or Python int's to_bytes().

PS: I have to update the compute shader too (note for me)

from moderngl.

Maybe I'm too much in the C++ mindset lately but if someone set Py_TPFLAGS_UNICODE_SUBCLASS but PyUnicode_AsUTF8 doesn't work on their class, I'd call that a bug in their class, not a bug in the library that assumed they could use PyUnicode* functions on classes that pass PyUnicode_Check. I certainly understand your hesitancy though and I would be happy to just add to_shader_source (good name choice, I like it) to my subclass if you weren't comfortable skating the thin ice.

from moderngl.

I think that too thats why i finally wrote "I think it is ok to use PyUnicode_Check then."

from moderngl.

You can install the master branch with pip install https://github.com/moderngl/moderngl/archive/refs/heads/master.zip

Please check if it works, we can release it afterwards.
Thank you for the report.

from moderngl.

Works perfectly for me! I tested using a str subclass as well as a to_shader_source wrapper and both worked. My IDE complains expected type str | bytes so perhaps type stubs/hints need to be updated to include a

class ConvertibleToShaderSource(Protocol):
    def to_shader_source(self) -> str:
        ...

and shader args in program hinted as str | bytes | ConvertibleToShaderSource | None. Other than that works great!

from moderngl.

updated stubs too

from moderngl.

For .py files A | B was introduced in 3.10 so if you want to use it in 3.7 you need to put from __future__ import annotations at the top. But for stubs I don't know if this is required.

from moderngl.

For stubs, your text editor's python's version matter, it can happen that it is parsed manually independently from what Python X.Y supports. I can add the from __future__ import annotations on the top it might help.
The .py file won't be updated as it will soon be removed entirely in #568

from moderngl.

Sounds good to me! Thanks for your help and closing as fixed.

from moderngl.

added from __future__ import annotations to the top of the pyi file thank you for recommending it.

from moderngl.