This is the script to perform changes proposed in T51219
Details
Diff Detail
- Repository
- rB Blender
- Branch
- arcpatch-D2678
- Build Status
Buildable 676 Build 676: arc lint + arc unit
Event Timeline
General comments.
I'm ok adding GWN_ to enum values and Gwn_ to data types.
Also ok shortening these words:
allocate --> alloc
primitive --> prim
Less ok shortening these:
vertex --> vert
attrib --> attr
Totally ok using abbreviations for variable names, just not for type names.
| gawain_refactor.py | ||
|---|---|---|
| 58 | Prefer MAX_VBO_CT = N, for valid range 0 to N-1. | |
| 105 | really should be MAX_VERTEX_ATTRIB_CT since it's 16 and valid values are 0 to 15 | |
| 118–121 | Original names are more accurate! These describe the conversion between VBO values and shader input values. Improved docs would help here, better than renaming. | |
| 140–143 | Yes to these abbreviations. | |
| 152–153 | I'm ok using the more common name of "index buffer". Gwn_IndexType | |
| 184 | was copying OpenGL primitive types, but as long as we're abbreviating... how about | |
| gawain_refactor.py | ||
|---|---|---|
| 58 | Using MAX_ as a prefix means not using GWN_ as a prefix. Its also significant that the maximum is associated with Batch and not some other part of the API. | |
| 118–121 | Even with docs, using a generic prefix here will result in a very long name: NORMALIZE_INT_TO_FLOAT becomes GWN_FETCH_NORMALIZE_INT_TO_FLOAT. Would GWN_FETCH_I32_TO_F32_NORMALIZE be acceptable? | |
| 152–153 | In that case shouldn't the API be renamed, eg: GWN_elemlist_build -> GWN_indexbuffer_build? | |
Rename conventions based on feedback from @Mike Erwin (merwin)
- attr -> attrib
- vert -> vertex
note that I still prefer the shorter abbreviations, since (vert/verts) -> (vertex/vertices), just more verbose without really helping readability.
The names are getting quite long, which I wanted to avoid, eg:
.GWN_vertbuffer_attr_fill_stride (or GWN_vbo_attr_fill_stride) becomes GWN_vertexbuffer_attrib_fill_stride.
| gawain_refactor.py | ||
|---|---|---|
| 118–121 | I32 and F32 are too specific. There are cases they don't match. We could make it GWN_FETCH_FLOAT GWN_FETCH_INT GWN_FETCH_CONVERT GWN_FETCH_NORMALIZE | |
| gawain_refactor.py | ||
|---|---|---|
| 165–166 | Thought about these after you brought them up: GWN_PrimType_class GWN_PrimType_belongs_to_class | |
| gawain_refactor.py | ||
|---|---|---|
| 168 | Done! 620516965b49 | |
Overall I like the idea of adding consistence to the API. But I think we are shortening things more than we really need. See some inline comments.
That said I'm not passionate either way.
| gawain_refactor.py | ||
|---|---|---|
| 61 | Same here, why use FMT instead of FORMAT? | |
| 110 | I personally don't see the benefit of shortening for the sake of it. I would use: At least for macros if not also for the function names. | |
| 121 | Typo, it should be GWN_FETCH_FLOAT | |
We can always tweak things a bit more later, if really needed, but better do main consistency/Blender-compatible naming asap, and current patch looks totally fine to me.
Small inline changes I had to do for OpenColorIO building (rB701a76769d4acb2dfda14a8fe2ef18ebd805d91c).
| gawain_refactor.py | ||
|---|---|---|
| 10 | + "intern/opencolorio" | |
| 264 | - if ext.lower() in {".c", ".cxx", ".cpp", ".h", ".hxx", ".hpp"}:
+ if ext.lower() in {".c", ".cc", ".cxx", ".cpp", ".h", ".hxx", ".hpp"}: | |