Gawain API general consistency RFC #51219

New Issue

Campbell Barton · 2017-04-15T10:35:57+02:00

Campbell Barton commented

2017-04-15 10:35:57 +02:00

For API's in intern or extern we normally aren't so fussed about their API's, however the way gawain is being used - it's functions and types are being included anywhere GPU is used, so it impacts nearly all drawing code.

Currently the API doesn't seem very consistent.

No prefix identifier for headers, constants, structs and functions

(this is something many C API's do OpenGL, Vulkan, FFMpeg & Python do, we also do this for most of Blender's API's)

Having a common prefix is important since C doesn't have name-spaces, Blender's editor code for example mixes many API's: windowing (GHOST)/drawing (gl)/audio (AUD)/Codecs (av)/input-methods (IME)/Blender Python (BPY)/File loading (BLO)... *and many more...*

When mixing logic from so many libraries means its important to be clear what library function calls are referencing.

It's also useful to do this for auto-completion, so you can type a prefix and see all the symbols available.

Public names (which get imported into most of Blender's drawing code) are very generic, eg:

`Batch`, `Attrib` `padding`, `IndexType`, `ElementList`, `VertexBuffer` ... with the chance these could conflict with other definitions.

Mixed camelCase / snake_case for function names, also within the same name, eg:

`ElementList_build`, `clear_AttribBinding` `VertexFormat_clear`, `VertexFormat_add_attrib`, `add_line_vertices`, `Batch_add_VertexBuffer`, `Batch_done_using_program`.

Gawain is a fairly small API, so it's not really so much work to give it some consistent conventions.

We could loosely follow Blender's conventions here: https://wiki.blender.org/index.php/Dev:Doc/Code_Style#Naming

eg:

  GWBatch, GWAttrib
  GW_attrib_binding_clear
  GW_batch_vertex_buffer_add
  GW_element_list_build
  GW_element_list_vertices_add_line
  GW_vertex_format_attrib_add

Or something closer to whats there now with a common prefix and more consistent word-ordering,
where (prefix+type are camel-case, the rest not).

  gwBatch, gwAttrib
  gwAttribBinding_clear
  gwBatch_vertex_buffer_add
  gwElementList_build
  gwElementList_vertices_add_line
  gwVertexFormat_attrib_add

... or something else.

(note that now might not be best time to apply such changes, but there is some chance it gets left as-is too if not handled soon enough).

Update, added utility to refactor the API: D2678

For API's in intern or extern we normally aren't so fussed about their API's, however the way gawain is being used - it's functions and types are being included anywhere GPU is used, so it impacts nearly all drawing code. Currently the API doesn't seem very consistent. - No prefix identifier for headers, constants, structs and functions *(this is something many C API's do OpenGL, Vulkan, FFMpeg & Python do, we also do this for most of Blender's API's)* ``` Having a common prefix is important since C doesn't have name-spaces, Blender's editor code for example mixes many API's: windowing (GHOST)/drawing (gl)/audio (AUD)/Codecs (av)/input-methods (IME)/Blender Python (BPY)/File loading (BLO)... *and many more...* ``` ``` When mixing logic from so many libraries means its important to be clear what library function calls are referencing. ``` ``` It's also useful to do this for auto-completion, so you can type a prefix and see all the symbols available. ``` - Public names (which get imported into most of Blender's drawing code) are very generic, eg: ``` `Batch`, `Attrib` `padding`, `IndexType`, `ElementList`, `VertexBuffer` ... with the chance these could conflict with other definitions. ``` - Mixed camelCase / snake_case for function names, also within the same name, eg: ``` `ElementList_build`, `clear_AttribBinding` `VertexFormat_clear`, `VertexFormat_add_attrib`, `add_line_vertices`, `Batch_add_VertexBuffer`, `Batch_done_using_program`. ``` ---- Gawain is a fairly small API, so it's not really so much work to give it some consistent conventions. We could loosely follow Blender's conventions here: https://wiki.blender.org/index.php/Dev:Doc/Code_Style#Naming eg: ``` GWBatch, GWAttrib GW_attrib_binding_clear GW_batch_vertex_buffer_add GW_element_list_build GW_element_list_vertices_add_line GW_vertex_format_attrib_add ``` Or something closer to whats there now with a common prefix and more consistent word-ordering, where (prefix+type are camel-case, the rest not). ``` gwBatch, gwAttrib gwAttribBinding_clear gwBatch_vertex_buffer_add gwElementList_build gwElementList_vertices_add_line gwVertexFormat_attrib_add ``` ... or something else. *(note that now might not be best time to apply such changes, but there is some chance it gets left as-is too if not handled soon enough)*. ---- Update, added utility to refactor the API: [D2678](https://archive.blender.org/developer/D2678)

Mike Erwin was assigned by Campbell Barton

2017-04-15 10:35:57 +02:00

Campbell Barton commented

2017-04-15 10:35:57 +02:00

Changed status to: 'Open'

Campbell Barton commented

2017-04-15 10:35:57 +02:00

Added subscribers: @ideasman42, @dfelinto, @fclem, @MikeErwin

Mike Erwin commented

2017-04-15 18:29:02 +02:00

Sure, I can explain the current naming conventions, and we can talk about whether/how to change them. Many people have been working with Gawain since October (and a few before that). I've tweaked the API based on their feedback to get to where we're at. The prefix subject has been brought up before but otherwise most people seem happy with the design.

Types use CamelCase, functions use snake_case. When part of a function name is a type, I use the ActualTypeName to make that 100% clear. It's an object-oriented C API so most functions look like TypeName_do_something(TypeName*, ...). The C++ version of Gawain looks cleaner because it doesn't have to spell out the type names.

The immediate mode functions all start with "imm" for a few reasons.

Begin, End, Vertex are common words and by themselves are not good function names
reminds coders that they're using immediate mode (a potential bottleneck)
imm functions work together, so coders know they can call immUniformColor and it will affect nearby immBegin/End draw calls

Enum values don't use a prefix because that would not add any info at the call site. For example when using COMP_F32 I don't care that the COMP types come from Gawain. GW_COMP_F32 is just more to visually parse. (I'm aware this a strange opinion)

Similar for gwVertexFormat_attrib_add. The prefix makes it harder to read at a glance, and adds no information.

The padding function is not really part of the external API. We can put that in a private header.

clear_AttribBinding should be AttribBinding_clear for consistency.

Could rename Attrib to VertexAttrib (more specific). Could also rename ElementList to IndexBuffer (more common). Naming conflicts are a hypothetical concern since existing names don't conflict with anything else. But I always try to choose the best names for things & am willing to rename for consistency or clarity.

Sure, I can explain the current naming conventions, and we can talk about whether/how to change them. Many people have been working with Gawain since October (and a few before that). I've tweaked the API based on their feedback to get to where we're at. The prefix subject has been brought up before but otherwise most people seem happy with the design. Types use CamelCase, functions use snake_case. When part of a function name is a type, I use the ActualTypeName to make that 100% clear. It's an object-oriented C API so most functions look like TypeName_do_something(TypeName*, ...). The C++ version of Gawain looks cleaner because it doesn't have to spell out the type names. The immediate mode functions all start with "imm" for a few reasons. - Begin, End, Vertex are common words and by themselves are not good function names - reminds coders that they're using immediate mode (a potential bottleneck) - imm functions work together, so coders know they can call immUniformColor and it will affect nearby immBegin/End draw calls Enum values don't use a prefix because that would not add any info at the call site. For example when using COMP_F32 I don't care that the COMP types come from Gawain. GW_COMP_F32 is just more to visually parse. *(I'm aware this a strange opinion)* Similar for gwVertexFormat_attrib_add. The prefix makes it harder to read at a glance, and adds no information. The padding function is not really part of the external API. We can put that in a private header. clear_AttribBinding should be AttribBinding_clear for consistency. Could rename Attrib to VertexAttrib (more specific). Could also rename ElementList to IndexBuffer (more common). Naming conflicts are a hypothetical concern since existing names don't conflict with anything else. But I always try to choose the best names for things & am willing to rename for consistency or clarity.

Mike Erwin commented

2017-04-15 19:25:25 +02:00

These functions are part of ElementListBuilder (aka IndexBufferBuilder):

void add_generic_vertex(ElementListBuilder*, unsigned v);
void add_point_vertex(ElementListBuilder*, unsigned v);
void add_line_vertices(ElementListBuilder*, unsigned v1, unsigned v2);
void add_triangle_vertices(ElementListBuilder*, unsigned v1, unsigned v2, unsigned v3);

ElementList (aka IndexBuffer) is immutable for fast drawing; the Builder adds vertex indices, validates, and converts to the final GPU-friendly form.

Possible naming schemes for this set of functions:

void IndexBufferBuilder_add_generic_vertex(IndexBufferBuilder*, unsigned v); // awkward name to do an awkward thing
void IndexBufferBuilder_add_point(IndexBufferBuilder*, unsigned v); // drop '_vertex'
void IndexBuffer_add_line(IndexBufferBuilder*, unsigned v1, unsigned v2); // drop 'Builder'
void IndexBuffer_add_triangle_vertices(IndexBufferBuilder*, unsigned v1, unsigned v2, unsigned v3); // keep '_vertices'

The final object you want is an IndexBuffer, but I wonder if the last two names are misleading. Thoughts?

These functions are part of ElementListBuilder (aka IndexBufferBuilder): ```lang=c void add_generic_vertex(ElementListBuilder*, unsigned v); void add_point_vertex(ElementListBuilder*, unsigned v); void add_line_vertices(ElementListBuilder*, unsigned v1, unsigned v2); void add_triangle_vertices(ElementListBuilder*, unsigned v1, unsigned v2, unsigned v3); ``` ElementList (aka IndexBuffer) is immutable for fast drawing; the Builder adds vertex indices, validates, and converts to the final GPU-friendly form. Possible naming schemes for this set of functions: ```lang=c void IndexBufferBuilder_add_generic_vertex(IndexBufferBuilder*, unsigned v); // awkward name to do an awkward thing void IndexBufferBuilder_add_point(IndexBufferBuilder*, unsigned v); // drop '_vertex' void IndexBuffer_add_line(IndexBufferBuilder*, unsigned v1, unsigned v2); // drop 'Builder' void IndexBuffer_add_triangle_vertices(IndexBufferBuilder*, unsigned v1, unsigned v2, unsigned v3); // keep '_vertices' ``` The final object you want is an IndexBuffer, but I wonder if the last two names are misleading. Thoughts?

Mike Erwin commented

2017-04-15 19:34:33 +02:00

Added subscribers: @LucaRood-3, @WillianPadovaniGermano, @define-private-public, @plasmasolutions, @JulianEisel

Mike Erwin commented

2017-04-15 19:34:33 +02:00

Adding people who have used Gawain quite a bit and/or have suggested API changes.

Julian Eisel commented

2017-04-15 19:39:26 +02:00

I think it's totally fine if an external library uses it's own code-style or naming conventions. However, Blender's calls to it should follow Blender's conventions. Especially in the case of a library that's accessed everywhere in Blender - like Gawain. We all know what Gawain is and what it does for us. For us, naming conventions may not be so important. But this code will likely see many years of aging, new devs will stumble over it, people who are familiar with the details of Gawain might move on to other, non-Blender things. So making it clear what functions and types do just by well though-out naming is crucial if you ask me.
As a matter of fact, I've been asked at some point to debug some immediate-mode replacement patches where it turned out to contain wrong usage of VertexCompType (using COMP_I32 instead of COMP_F32 or so). Reaction of the patch author was like: "How the hell should I know what this is doing, the names are just useless and there is no documentation. I just copy & pasted it!". Rephrasing from memory here, but this actually happened and I'm not exaggerating even ;)

So I guess we don't want to make Gawain follow Blender's conventions since it's purpose is not being designed for Blender only (right?). My suggestion would be to have Blender versions of Gawain's headers in soure/blender/gpu, actually there are already some. It would wrap all functions and types, e.g.

/**
 * \file blender/gpu/GPU_immediate_mode.h
 */

#include "gawain/immediate.h"


typedef struct VertexFormat GPU_vertex_format; /* member access usually isn't needed, but if it is, access via getters/setters */

- define GPU_immediate_vertex_2f immVertex2f
- define GPU_immediate_uniform_m4_set immUniformMatrix4fv
...

For enum items it's not so easy since they have to be kept in sync with Gawain carefully. I think it's an acceptable burden though, a small .py script could automate that to some degree.

Note on GW_ vs. GPU_ prefix, I don't have a too strong opinion here, but GPU_ communicates much better that this is related to drawing than GW_. If we choose to go with a solution similar to what I proposed, this benefit comes at no cost.

I think it's totally fine if an external library uses it's own code-style or naming conventions. However, Blender's calls to it should follow Blender's conventions. Especially in the case of a library that's accessed everywhere in Blender - like Gawain. We all know what Gawain is and what it does for us. For us, naming conventions may not be so important. But this code will likely see many years of aging, new devs will stumble over it, people who are familiar with the details of Gawain might move on to other, non-Blender things. So making it clear what functions and types do just by well though-out naming is crucial if you ask me. As a matter of fact, I've been asked at some point to debug some immediate-mode replacement patches where it turned out to contain wrong usage of `VertexCompType` (using `COMP_I32` instead of `COMP_F32` or so). Reaction of the patch author was like: "How the hell should I know what this is doing, the names are just useless and there is no documentation. I just copy & pasted it!". Rephrasing from memory here, but this actually happened and I'm not exaggerating even ;) So I guess we don't want to make Gawain follow Blender's conventions since it's purpose is not being designed for Blender only (right?). My suggestion would be to have Blender versions of Gawain's headers in `soure/blender/gpu`, actually there are already some. It would wrap all functions and types, e.g. ``` /** * \file blender/gpu/GPU_immediate_mode.h */ #include "gawain/immediate.h" typedef struct VertexFormat GPU_vertex_format; /* member access usually isn't needed, but if it is, access via getters/setters */ - define GPU_immediate_vertex_2f immVertex2f - define GPU_immediate_uniform_m4_set immUniformMatrix4fv ... ``` For enum items it's not so easy since they have to be kept in sync with Gawain carefully. I think it's an acceptable burden though, a small .py script could automate that to some degree. Note on `GW_` vs. `GPU_` prefix, I don't have a too strong opinion here, but `GPU_` communicates *much* better that this is related to drawing than `GW_`. If we choose to go with a solution similar to what I proposed, this benefit comes at no cost.

Mike Erwin commented

2017-04-15 20:24:42 +02:00

In #51219#428306, @JulianEisel wrote:
people who are familiar with the details of Gawain might move on to other, non-Blender things.

I've been here for 7 years so far, and plan to be around a while :) But good point, it needs to stand on its own whether or not someone is around to explain it.

So making it clear what functions and types do just by well though-out naming is crucial if you ask me.

I totally agree! The current names are my best attempt at that.

As a matter of fact, I've been asked at some point to debug some immediate-mode replacement patches where it turned out to contain wrong usage of VertexCompType (using COMP_I32 instead of COMP_F32 or so). Reaction of the patch author was like: "How the hell should I know what this is doing, the names are just useless and there is no documentation. I just copy & pasted it!". Rephrasing from memory here, but this actually happened and I'm not exaggerating even ;)

Don't copy/paste code and expect it to work :/ But seriously, I've also encountered this in code review, and better documentation would really help. There is some scattered in the wiki and G docs. I just haven't had time yet to write a comprehensive manual.

it's purpose is not being designed for Blender only (right?).

Right, the purpose is to handle common GPU tasks & to some extent abstract away the differences between native graphics APIs (OpenGL, Vulkan). Blender is the first (& only so far) user. The code in intern/gawain is "with modifications specific to integration with Blender" because it should be a good match to whatever Blender needs. But I don't want to bend it so much that it becomes a Blender-only library.

My suggestion would be to have Blender versions of Gawain's headers in soure/blender/gpu, actually there are already some.

That's sort of true. Those files are how the rest of Blender accesses Gawain, plus they add some Blender-specific functions that work with Gawain but are not strictly part of it. Things like immBindBuiltinProgram & immUniformThemeColor, since Gawain doesn't know about Blender's built-in shaders or theme colors (mostly for GPL compliance).

It would wrap all functions and types ...

All of that looks terrible to me. More complicated and harder to read. But I'm open to further discussion! Not endless bikeshedding, but it is important to get this stuff right.

> In #51219#428306, @JulianEisel wrote: > people who are familiar with the details of Gawain might move on to other, non-Blender things. I've been here for 7 years so far, and plan to be around a while :) But good point, it needs to stand on its own whether or not someone is around to explain it. > So making it clear what functions and types do just by well though-out naming is crucial if you ask me. I totally agree! The current names are my best attempt at that. > As a matter of fact, I've been asked at some point to debug some immediate-mode replacement patches where it turned out to contain wrong usage of `VertexCompType` (using `COMP_I32` instead of `COMP_F32` or so). Reaction of the patch author was like: "How the hell should I know what this is doing, the names are just useless and there is no documentation. I just copy & pasted it!". Rephrasing from memory here, but this actually happened and I'm not exaggerating even ;) Don't copy/paste code and expect it to work :/ But seriously, I've also encountered this in code review, and better documentation would really help. There is some scattered in the wiki and G docs. I just haven't had time yet to write a comprehensive manual. > it's purpose is not being designed for Blender only (right?). Right, the purpose is to handle common GPU tasks & to some extent abstract away the differences between native graphics APIs (OpenGL, Vulkan). Blender is the first (& only so far) user. The code in intern/gawain is "with modifications specific to integration with Blender" because it should be a good match to whatever Blender needs. But I don't want to bend it so much that it becomes a Blender-only library. > My suggestion would be to have Blender versions of Gawain's headers in `soure/blender/gpu`, actually there are already some. That's sort of true. Those files are how the rest of Blender accesses Gawain, plus they add some Blender-specific functions that work with Gawain but are not strictly part of it. Things like immBindBuiltinProgram & immUniformThemeColor, since Gawain doesn't know about Blender's built-in shaders or theme colors (mostly for GPL compliance). > It would wrap all functions and types ... All of that looks terrible to me. More complicated and harder to read. But I'm open to further discussion! Not endless bikeshedding, but it is important to get this stuff right.

Campbell Barton commented

2017-04-18 06:46:56 +02:00

@MikeErwin, agree wrapping Gawain isn't a net gain but surprised you're against using a common prefix.

OpenGL and Vulkan both do this for good reason, without name-spaces it becomes confusing to read code, a prefix is a convenient way to know a function call addresses Gawain and not one of the many other libraries.

Even though Blender's drawing code should primarily just handle drawing (not other random tasks), we end up mixing in drawing code for operator-draw-callbacks, widgets, transform callbacks... and areas that use other libraries, meaning that short terms aren't obviously related to drawing (the term Batch in transform code for example could refer to Batch transformations ... for eg).

I'm surprised this is even an issue of contention (since this is such a common convention in C API's including Blender's own code), I've updated the main task giving some reasons to do this.

Further, it would be good to hear from other Blender developers who are using Gawain.

@MikeErwin, agree wrapping Gawain isn't a net gain but surprised you're against using a common prefix. OpenGL and Vulkan both do this for good reason, without name-spaces it becomes confusing to read code, a prefix is a convenient way to know a function call addresses Gawain and not one of the many other libraries. Even though Blender's drawing code should primarily just handle drawing (not other random tasks), we end up mixing in drawing code for operator-draw-callbacks, widgets, transform callbacks... and areas that use other libraries, meaning that short terms aren't obviously related to drawing *(the term Batch in transform code for example could refer to Batch transformations ... for eg).* ---- I'm surprised this is even an issue of contention *(since this is such a common convention in C API's including Blender's own code)*, I've updated the main task giving some reasons to do this. Further, it would be good to hear from other Blender developers who are using Gawain.

Mike Erwin commented

2017-04-18 08:59:55 +02:00

In #51219#428994, @ideasman42 wrote:
@MikeErwin ... surprised you're against using a common prefix.

I'm not totally against prefixes, was just explaining why I opted for the current naming scheme. Right now it reads like an English speaker talking about graphics programming. Start adding prefixes and it reads like a library with a silly name talking about itself :) Even the OpenGL spec drops its own "GL_" and "gl" prefixes, leading to a more enjoyable read. Doing it well / tastefully is my main concern.

Let's try some variants of the most widespread type:

VertexFormat
GWNVertexFormat <-- my least favorite
GW_VertexFormat
GWN_VertexFormat <-- not bad
Gwn_VertexFormat <-- kinda like this one
GwnVertexFormat

@JulianEisel suggested the GPU prefix (which I also like) but that's already used for Blender's GPU module, which is separate code. The first time someone sees a type or function with this prefix they'll ask WTF is GWN, go look it up or ask on IRC and from then on know that it has something to do with GPU drawing. People know what Cycles is even though it doesn't say "high quality renderer" right there in the name.

BTW for headers my intention was to #include "gawain/some_feature.h", but my CMake skills are too lame to make that work! Only a few files in the GPU module need to include things directly. The rest of Blender simply uses the GPU module.

I think the "imm" prefix used by the immediate mode API is fine. Gawain implements most of it, then GPU adds many useful functions on top of that. When working on drawing code, it doesn't matter where the functions come from, just that they all work together.

Even though Blender's drawing code should primarily just handle drawing (not other random tasks), we end up mixing in drawing code for operator-draw-callbacks, widgets, transform callbacks... and areas that use other libraries, meaning that short terms aren't obviously related to drawing (the term Batch in transform code for example could refer to Batch transformations ... for eg).

That's a great example. Maybe DrawBatch is a better name (draw as adjective, not verb). Or maybe GwnBatch is enough to indicate "oh it's part of the Gawain library, has something to do with GPU drawing". I'd like to know what other graphics software & game engines call this.

Further, it would be good to hear from other Blender developers who are using Gawain.

Yes, speak up people! What do you think of these issues? Do any of the suggested prefixes stand out as good or bad?

> In #51219#428994, @ideasman42 wrote: > @MikeErwin ... surprised you're against using a common prefix. I'm not totally against prefixes, was just explaining why I opted for the current naming scheme. Right now it reads like an English speaker talking about graphics programming. Start adding prefixes and it reads like a library with a silly name talking about *itself* :) Even the OpenGL spec drops its own "GL_" and "gl" prefixes, leading to a more enjoyable read. Doing it well / tastefully is my main concern. Let's try some variants of the most widespread type: ``` VertexFormat GWNVertexFormat <-- my least favorite GW_VertexFormat GWN_VertexFormat <-- not bad Gwn_VertexFormat <-- kinda like this one GwnVertexFormat ``` @JulianEisel suggested the GPU prefix (which I also like) but that's already used for Blender's GPU module, which is separate code. The first time someone sees a type or function with this prefix they'll ask WTF is GWN, go look it up or ask on IRC and from then on know that it has something to do with GPU drawing. People know what Cycles is even though it doesn't say "high quality renderer" right there in the name. BTW for headers my intention was to #include "gawain/some_feature.h", but my CMake skills are too lame to make that work! Only a few files in the GPU module need to include things directly. The rest of Blender simply uses the GPU module. I think the "imm" prefix used by the immediate mode API is fine. Gawain implements most of it, then GPU adds many useful functions on top of that. When working on drawing code, it doesn't matter where the functions come from, just that they all work together. > Even though Blender's drawing code should primarily just handle drawing (not other random tasks), we end up mixing in drawing code for operator-draw-callbacks, widgets, transform callbacks... and areas that use other libraries, meaning that short terms aren't obviously related to drawing *(the term Batch in transform code for example could refer to Batch transformations ... for eg).* That's a great example. Maybe DrawBatch is a better name (draw as adjective, not verb). Or maybe GwnBatch is enough to indicate "oh it's part of the Gawain library, has something to do with GPU drawing". I'd like to know what other graphics software & game engines call this. > Further, it would be good to hear from other Blender developers who are using Gawain. Yes, speak up people! What do you think of these issues? Do any of the suggested prefixes stand out as good or bad?

Campbell Barton commented

2017-04-18 09:23:28 +02:00

@MikeErwin, the prefix is just a terse abbreviation/sign, initially developers will need to look up a symbol to see what it does (no need to ask in IRC), once a developer does this, they see it's from the garwain module - and remember it, as we already do with DRW/GPU/GL/GHOST/BLI/Python... etc.

The point is that once you remember GW is for garwain, you don't have to keep memory of every function name in the Garwain API to see at a glance what part of the API a function is from (this is what I mean by acting as name-spaces for C code). With the further advantage that when you can't remember a symbol name, auto-completion can show a list once you type in the prefix.

Exact details don't bother me so much, I'd use GW instead of GWN (GW is the start of each syllable, short and unique in Blender). Notice Vulkan uses VK not VKN

Re: Batch / DrawBatch / GwBatch... one of the advantages of using a prefix means you don't need to use the term ...Draw.... In the context of a graphics API, the term batch is then enough (maybe GwBatchDraw is a good idea if we have GwBatchSomeOtherOperation later... it's just an example, possibly batch-draw is better).

@MikeErwin, the prefix is just a terse abbreviation/sign, initially developers will need to look up a symbol to see what it does (no need to ask in IRC), once a developer does this, they see it's from the garwain module - and remember it, as we already do with DRW/GPU/GL/GHOST/BLI/Python... etc. The point is that once you remember GW is for garwain, you don't have to keep memory of every function name in the Garwain API to see at a glance what part of the API a function is from *(this is what I mean by acting as name-spaces for C code)*. With the further advantage that when you can't remember a symbol name, auto-completion can show a list once you type in the prefix. ---- Exact details don't bother me so much, I'd use `GW` instead of `GWN` (GW is the start of each syllable, short and unique in Blender). *Notice Vulkan uses `VK` not `VKN`* Re: `Batch` / `DrawBatch` / `GwBatch`... one of the advantages of using a prefix means you don't _need_ to use the term `...Draw...`. In the context of a graphics API, the term batch is then enough *(maybe `GwBatchDraw` is a good idea if we have `GwBatchSomeOtherOperation` later... it's just an example, possibly batch-draw is better)*.

Bastien Montagne commented

2017-04-18 09:30:30 +02:00

Added subscriber: @mont29

Bastien Montagne commented

2017-04-18 09:30:30 +02:00

I think using a prefix is kind of mandatory here, yes. Which one it is and which “style” it uses I do not care much tbh, as already said extern libraries are free to follow their own code style. Personally, I tend to like GWN_ the most (also closer to our own BLI_, BKE_, etc. ;) ).

But again, no strong opinion here, as long as we can see immediately which calls belong to Gawain. Nuff said. :)

I think using a prefix is kind of mandatory here, yes. Which one it is and which “style” it uses I do not care much tbh, as already said extern libraries are free to follow their own code style. Personally, I tend to like `GWN_` the most (also closer to our own `BLI_`, `BKE_`, etc. ;) ). But again, no strong opinion here, as long as we can see immediately which calls belong to Gawain. Nuff said. :)

Campbell Barton commented

2017-04-18 09:47:36 +02:00

For naming we should consider different types of symbols.

I'd suggest the following conventions:

GW_BATCH_FOO_BAR for constants/enums/macros.
GwBatchFooBar for structs.
GW_batch_foo_bar or gwBatchFooBar for functions.

... using the term Batch when it could be replaces for VertexFormat, ElementList or other parts of the API.

This is basically Blender's existing naming convention, where gwBatchFooBar would be the only exception.

I don't have real strong opinion on GW/GWN, all examples above could use GWN/Gwn instead.

For naming we should consider different types of symbols. I'd suggest the following conventions: - `GW_BATCH_FOO_BAR` for constants/enums/macros. - `GwBatchFooBar` for structs. - `GW_batch_foo_bar` or `gwBatchFooBar` for functions. ... using the term `Batch` when it could be replaces for `VertexFormat`, `ElementList` or other parts of the API. This is basically Blender's existing naming convention, where `gwBatchFooBar` would be the only exception. I don't have real strong opinion on GW/GWN, all examples above could use GWN/Gwn instead.

Campbell Barton commented

2017-04-18 23:06:11 +02:00

Added utility to update the API: P453

Added utility to update the API: [P453](https://archive.blender.org/developer/P453.txt)

Mike Erwin commented

2017-05-05 17:48:28 +02:00

Some notes on specifics, so we can use P453 as a base for discussion.

I like GW_BATCH_READY_TO_DRAW and the other batch phase enums. The GW_FETCH names I'm mixed about -- neither set of names is perfect. At least with the FETCH prefix we get a short list of the options from autocomplete.

convert_prim_type_to_gl function should be private. I'll get on that.

GW_vertbuffer_attr_stride should be GW_vertbuffer_attr_fill_stride.

GW_batch_program_set
GW_batch_program_use_begin
GW_batch_program_use_end

^-- I like these

GW_elemlist_vert_add
GW_elemlist_line_verts_add
GW_elemlist_tri_verts_add

^-- these are worse than the names they replace
No autocompletion and the word "point" is meaningful here. Prefer these:

GW_elemlist_add_point_vert
GW_elemlist_add_line_verts
GW_elemlist_add_tri_verts (or triangle_verts)

There's a duplicate GW_PRIM_LINE_STRIP_ADJACENCY.

Some notes on specifics, so we can use [P453](https://archive.blender.org/developer/P453.txt) as a base for discussion. I like `GW_BATCH_READY_TO_DRAW` and the other batch phase enums. The GW_FETCH names I'm mixed about -- neither set of names is perfect. At least with the FETCH prefix we get a short list of the options from autocomplete. `convert_prim_type_to_gl` function should be private. I'll get on that. `GW_vertbuffer_attr_stride` should be GW_vertbuffer_attr_**fill**_stride. ``` GW_batch_program_set GW_batch_program_use_begin GW_batch_program_use_end ``` ^-- I like these ``` GW_elemlist_vert_add GW_elemlist_line_verts_add GW_elemlist_tri_verts_add ``` ^-- these are worse than the names they replace No autocompletion and the word "point" is meaningful here. Prefer these: ``` GW_elemlist_add_point_vert GW_elemlist_add_line_verts GW_elemlist_add_tri_verts (or triangle_verts) ``` There's a duplicate `GW_PRIM_LINE_STRIP_ADJACENCY`.

Mike Erwin commented

2017-05-05 19:40:38 +02:00

Bigger picture, I prefer function names mostly as they are. That's why I chose them! Some of the P453 changes are better though, so we can pick & choose.

I prefer GWN_ over GW_, even though GW-BASIC introduced me to the world of programming. It sounds more like "Gawain" when I read it.

GWN_ENUM_VALUE (similar for constants & macros)
GWN_stand_alone_func
GWN_DataType_func

For data types (struct, enum), one of these?
GWN_DataType <-- same prefix for everything
Gwn_DataType
GwnDataType <-- looks least ugly in function parameter list

Other graphics APIs for comparison:

OpenGL uses GL_ for constants, GL for typedefs, gl for functions.
Vulkan uses VK_ for constants, Vk for data types, vk for functions.
Metal uses MTL for constants, MTL for data types, no prefix for functions (OO language)
3dfx Glide used GR_ for constants, Gr for data types, gr for functions.

Bigger picture, I prefer function names mostly as they are. That's why I chose them! Some of the [P453](https://archive.blender.org/developer/P453.txt) changes are better though, so we can pick & choose. I prefer `GWN_` over `GW_`, even though GW-BASIC introduced me to the world of programming. It sounds more like "Gawain" when I read it. GWN_ENUM_VALUE (similar for constants & macros) GWN_stand_alone_func GWN_DataType_func For data types (struct, enum), one of these? GWN_DataType <-- same prefix for everything Gwn_DataType GwnDataType <-- looks least ugly in function parameter list Other graphics APIs for comparison: - OpenGL uses GL_ for constants, GL for typedefs, gl for functions. - Vulkan uses VK_ for constants, Vk for data types, vk for functions. - Metal uses MTL for constants, MTL for data types, no prefix for functions (OO language) - 3dfx Glide used GR_ for constants, Gr for data types, gr for functions.

Campbell Barton commented

2017-05-05 22:13:38 +02:00

@MikeErwin, thanks for the feedback, updated D2678:

I was going for "unique prefix and short names", your suggestions are ~1-2 chars longer, but dont see any issues with that.

Use GWN_ prefix for functions/macros (@mont29 also prefers).
Use Gwn_DataType for structs (the alternatives are too similar to function names IMHO, unless we used lowercase prefix for functions).

Also...

Use GWN_elemlist_add_#() ... instead of _#_add() (also for GWN_elemlist_add_generic_vert).
Corrected GW_vertbuffer_attr_fill_stride
Use short type-identifiers for GWN_FETCH_* matching GWN_COMP_* (agree these are a bit awkward, but GWN_FETCH_NORMALIZE_INT_TO_FLOAT is too verbose)
use TRI abbreviation for constants (functions used tri, constants TRIANGLES, in context I think its clear what is meant by GWN_PRIM_TRI_STRIP).
Noted convert_prim_type_to_gl as private.

@MikeErwin, thanks for the feedback, updated [D2678](https://archive.blender.org/developer/D2678): I was going for *"unique prefix and short names"*, your suggestions are ~1-2 chars longer, but dont see any issues with that. - Use `GWN_` prefix for functions/macros (@mont29 also prefers). - Use `Gwn_DataType` for structs *(the alternatives are too similar to function names IMHO, unless we used lowercase prefix for functions).* Also... - Use `GWN_elemlist_add_#()` ... instead of `_#_add()` (also for `GWN_elemlist_add_generic_vert`). - Corrected `GW_vertbuffer_attr_fill_stride` - Use short type-identifiers for `GWN_FETCH_*` matching `GWN_COMP_*` *(agree these are a bit awkward, but `GWN_FETCH_NORMALIZE_INT_TO_FLOAT` is too verbose)* - use `TRI` abbreviation for constants *(functions used `tri`, constants `TRIANGLES`, in context I think its clear what is meant by `GWN_PRIM_TRI_STRIP`).* - Noted `convert_prim_type_to_gl` as private.

Dalai Felinto commented

2017-06-11 12:26:21 +02:00

@ideasman42 are all the critical topics addressed/committed already?

Campbell Barton commented

2017-06-11 13:40:25 +02:00

@dfelinto I think I've made most of the suggestions, but mike wanted to upload his own proposal and send to bf-viewport, that was some weeks back though.

@MikeErwin, I've made some updates to this proposal, could you check over it? Or if you propose something totally different could you submit that as an alternative? (copy-pasting this patch and find-replace shouldn't take too long,

@dfelinto I think I've made most of the suggestions, but mike wanted to upload his own proposal and send to bf-viewport, that was some weeks back though. @MikeErwin, I've made some updates to this proposal, could you check over it? Or if you propose something totally different could you submit that as an alternative? (copy-pasting this patch and find-replace shouldn't take too long,

Campbell Barton commented

2017-06-28 13:14:13 +02:00

Changed status from 'Open' to: 'Resolved'

Campbell Barton closed this issue

2017-06-28 13:14:13 +02:00

Campbell Barton commented

2017-06-28 13:14:35 +02:00

D2678 has been applied, closing

[D2678](https://archive.blender.org/developer/D2678) has been applied, closing

Satish Goda commented

2017-07-18 11:39:12 +02:00

Added subscriber: @satishgoda1

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Gawain API general consistency RFC #51219