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

Support for custom shader attribute names #937

Open
fatcerberus opened this Issue Oct 12, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@fatcerberus
Copy link
Contributor

fatcerberus commented Oct 12, 2018

Currently the default shader attributes and uniforms are hardcoded with names beginning with al_.

As I’m in the process of porting miniSphere to the Web (https://github.com/fatcerberus/oozaru), I found that I needed to use the Allegro attribute names in my WebGL backend in order to ensure all shaders are cross-compatible with the desktop implementation. This is less than ideal; I would like the ability to use custom names for these attributes if desired. Otherwise I have to use the OpenGL API directly and lose all the convenience of Allegro’s helpers.

I’m not sure what the best API to expose this functionality is, so I’m posting this issue to hopefully get some discussion going.

@elias-pschernig

This comment has been minimized.

Copy link
Member

elias-pschernig commented Oct 13, 2018

Could we just add a string to ALLEGRO_VERTEX_ELEMENT which is the GLSL name of the attribute?

@elias-pschernig

This comment has been minimized.

Copy link
Member

elias-pschernig commented Oct 13, 2018

This is where they are assigned:

varlocs->pos_loc = glGetAttribLocation(program, ALLEGRO_SHADER_VAR_POS);

So right now e.g. ALLEGRO_SHADER_VAR_POS is the fixed string "al_pos" - instead that just would be the default but we would assign the name from the vertex declaration. I suppose for your use case it actually would be enough to have something like:

al_set_shader_attribute_name(ALLEGRO_SHADER_VAR_POS, "my_xyz_attribute");

And it would just globally change the names Allegro uses in all shaders.

@fatcerberus

This comment has been minimized.

Copy link
Contributor Author

fatcerberus commented Oct 13, 2018

Note that it’s not just the attributes - there are a few hardcoded uniforms as well, such as al_tex.

@elias-pschernig

This comment has been minimized.

Copy link
Member

elias-pschernig commented Oct 13, 2018

Well, al_tex is the texture Allegro is using for its own drawing, for example al_draw_bitmap might internally draw two triangles with that texture. You can already assign a bitmap to any other uniform like "my_tex" though if you want. I still don't think it would be bad if we allow to rename them - but I don't really see the use case now.

@fatcerberus

This comment has been minimized.

Copy link
Contributor Author

fatcerberus commented Oct 13, 2018

That's true for the uniforms, although in some cases replacing them with custom uniforms requires, if not using the OpenGL API directly, a lot more work with the Allegro API than just leaving the built-in ones in place. For example using al_projview_matrix in a shader means Allegro can handle a lot of the matrix multiplication for me allowing me to use conveniences like al_use_projection_transform().

For al_tex specifically, it's used internally, true, but it's also used as the default binding for the bitmap passed, to, e.g. al_draw_vertex_buffer(). Basically I want my code to still look "Allegro-idiomatic" without taking a hard dependency on Allegro semantics in my public-facing API specification. I do realize I'm probably being too picky now with that, so I apologize. 😸

@SiegeLord

This comment has been minimized.

Copy link
Member

SiegeLord commented Oct 13, 2018

al_set_shader_allegro_hooks(ALLEGRO_SHADER*, const char* al_tex, ...)?

@fatcerberus

This comment has been minimized.

Copy link
Contributor Author

fatcerberus commented Oct 13, 2018

For the attributes (al_pos, etc.) specifically, I was thinking that since we already have an API for setting the layout of a vertex buffer (al_create_vertex_decl), it might make more sense to allow each element to include an attribute name as well (@elias-pschernig's first suggestion above).

For the uniform names, al_set_shader_hooks or whatever would probably be the simplest approach, though it's worth noting that adding new parameters to that later breaks source compatibility. To avoid that we could use an enum:

al_set_default_shader_hook(shader, ALLEGRO_SHADER_TEX, "u_texture");
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.