Page MenuHome

Gawain refactor
ClosedPublic

Authored by Campbell Barton (campbellbarton) on May 19 2017, 7:23 AM.

Details

Summary

This is the script to perform changes proposed in T51219

Diff Detail

Repository
rB Blender
Branch
arcpatch-D2678
Build Status
Buildable 699
Build 699: 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
Gwn_IndexBuffer
Gwn_IndexBufferBuilder

184

was copying OpenGL primitive types, but as long as we're abbreviating... how about
GWN_PRIM_LINE_STRIP_ADJ

Campbell Barton (campbellbarton) marked an inline comment as done.

Minor edits, rename *_MAX to *_MAX_LEN to avoid off-by-one confusion.

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.

  • Use IndexBuffer name
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
58

GWN_BATCH_MAX_VBO_CT

Gawain uses count and size in most places, length not so much (just for strings).

75

Drop the _get

105

GWN_MAX_VERTEX_ATTRIB_CT

gawain_refactor.py
165–166

Thought about these after you brought them up:

GWN_PrimType_class
GWN_PrimType_belongs_to_class
  • Some changes from own preferences, use some shorter names
  • Remove fmt, its a bit confusing
gawain_refactor.py
168

Done! 620516965b49

  • remove private function
  • Use less specific identifiers
  • Merge branch '28' into arcpatch-D2678
  • Update for changes in 2.8
  • Merge branch '28' into arcpatch-D2678
  • FMT -> FORMAT

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:
GWN_VERT_ATTR_NAME_AVERAGE_LEN, GWN_VERT_ATTR_NAMES_BUFFER_LEN

At least for macros if not also for the function names.

121

Typo, it should be

GWN_FETCH_FLOAT
  • Merge branch '28' into arcpatch-D2678
  • AVG -> AVERAGE
Campbell Barton (campbellbarton) marked 2 inline comments as done.
This revision is now accepted and ready to land.Jun 19 2017, 11:07 AM

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
9
+ "intern/opencolorio"
263
- if ext.lower() in {".c", ".cxx", ".cpp", ".h", ".hxx", ".hpp"}:
+ if ext.lower() in {".c", ".cc", ".cxx", ".cpp", ".h", ".hxx", ".hpp"}: