cglm icon indicating copy to clipboard operation
cglm copied to clipboard

Remove implementation files, make same API for inlining and shared library

Open tylov opened this issue 5 years ago • 3 comments

  • Remove all include/cglm/call.h, include/cglm/call/.h and src/.c files, while keeping option to compile and use CGLM as a shared library.
  • ONLY ONE API, and switch between compiling it inline or using shared library by just defining CGLM_LIB.
  • Simplify maintenance and reduce number of files. Details:
  • Build library by compiling src/cglm.c. This defines CGLM_DLL
  • Use library by defining -DCGLM_LIB and link with created lib (dll or so).

FIXES/changes in include/cglm/common.h:

  • '__declspec(...)' was only enabled for Visual Studio, but all major compilers on windows uses this, including MinGW / Cygwin GCC, Tiny C, etc.
  • 'attribute((visibility("default")))' was used for both import and export for all other compilers than VC, but should generally only be enabled for export with GCC.
  • 'static inline __attribute((always_inline))' was used for all compilers other than VC, should generally be used for GCC, otherwise use 'static inline'.

Note: It should be trivial to extend common.h so that you could e.g. define CGLM_STATIC_LIB as a third compilation option in addition to inline and shared library.

Tested on windows gcc (tdm64) 9.2, Visual studio 2019, Tiny C. Code changes were done in Notepad++ using the regular expressions below (on all open files). Only the first function in io.h and the tests needed to be edited manually.

find 1: CGLM_INLINE repl 1: CGLM_DECL

find 2: ) {\r\n(#if) repl 2: ) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n{\r\n$1

find 3: ) {\r\n( [^ ]) repl 3: ) CGLM_ENDD\r\n#ifndef CGLM_LIB\r\n{\r\n$1

find 4: ^}\r\n\r\n repl 4: }\r\n#endif\r\n\r\n

tylov avatar Apr 11 '20 16:04 tylov

Hi @tylo-work

Thank you for contributing to cglm 🤗


I don't know, this is a GOOD IDEA but brings a HUGE change to the design and code base and conflicts with the idea that provide both inline and compile version.

For instance users can use both glm_mat4_mul() (inline) and glmc_mat4_mul() (library-call) at the same time. glm_ is always inline (tries to be) and glmc_ is always library-called / compiled version. So there is no confusing.

I don't know, let's get more reviews, more feedbacks here.

recp avatar Apr 13 '20 18:04 recp

No problem, I see the issues that you mention, yes it does remove existing API, and you can no longer pick which function to call from library. Ideally though, one should not need to pick and choose functions to be called from library vs inline, so “small” functions should always be defined inline, and the longer should be library functions when you choose this configuration.

I have started creating my own personal smaller c-library (using some of your code and c++ glm), so I can make it perfect for my needs.

Btw, I have a suggestion for new swizzle functions. Speed should be around as fast as yours, but this is more user friendly and flexible, imo:

CGLM_INLINE void glm_swizzle4(vec4 v, const char *swz, vec4 dest) { dest[0] = v[(swz[0] - 'x') & 3]; dest[1] = v[(swz[1] - 'x') & 3]; dest[2] = v[(swz[2] - 'x') & 3]; dest[3] = v[(swz[3] - 'x') & 3]; }

CGLM_INLINE void glm_swizzle3(vec4 v, const char *swz, vec3 dest) { dest[0] = v[(swz[0] - 'x') & 3]; dest[1] = v[(swz[1] - 'x') & 3]; dest[2] = v[(swz[2] - 'x') & 3]; }

CGLM_INLINE void glm_swizzle2(vec4 v, const char *swz, vec2 dest) { dest[0] = v[(swz[0] - 'x') & 3]; dest[1] = v[(swz[1] - 'x') & 3]; } // Examples: vec4 v1 = {1, 2, 3, 4}, v2; vec3 u1, u2; vec2 p; glm_swizzle4(v1, “wxyy”, v2); glm_swizzle3(v2, “xyw”, u1); glm_swizzle3(u1, “zyx”, u2); glm_swizzle2(u2, “zx”, p); glm_swizzle4(p, “xxyy”, v2);

Ps: Your following defines are wrong, should be (3, 2, 1, 0), although you won’t need them much if you use my swizzle functions 😉

#define GLM_WZYX GLM_SHUFFLE4(0, 1, 2, 3) #define GLM_ZYX GLM_SHUFFLE3(0, 1, 2)

Cheers,

Tyge Løvset Seniorforsker Senior Researcher

+47 932 67 779

Fantoftvegen 38, 5072 Bergen, Norway

[cid:norce_426cc920-64ab-4893-95d0-c29d5dab670c.png]

NORCE Norwegian Research Centre AS norceresearch.nohttps://www.norceresearch.no/ From: Recep Aslantas [email protected] Sent: Monday, April 13, 2020 20:26 To: recp/cglm [email protected] Cc: Tyge Løvset [email protected]; Mention [email protected] Subject: Re: [recp/cglm] Remove implementation files, make same API for inlining and shared library (#135)

Hi @tylo-workhttps://github.com/tylo-work

Thank you for contributing to cglm 🤗


I don't know, this is a GOOD IDEA but brings a HUGE change to the design and code base and conflicts with the idea that provide both inline and compile version.

For instance users can use both glm_mat4_mul() (inline) and glmc_mat4_mul() (library-call) at the same time. glm_ is always inline (tries to be) and glmc_ is always library-called / compiled version. So there is no confusing.

I don't know, let's get more reviews, more feedbacks here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/recp/cglm/pull/135#issuecomment-613027574, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AOLYYGX3X4B5GZKFUP5X5V3RMNKJ3ANCNFSM4MGBTBGQ.

tylov avatar Apr 14 '20 09:04 tylov

I think this PR must stay open if you like to get more feedbacks from others. In the future we may want to bring this changes... Feel free to contribute cglm for other bug fixes and features... ;)

Some places may require some functions like glm_mat4_mul/glm_mul ... to be inline to get rid of function call overhead, in critical sections to get max performance. Some places may not require that to save binary size. Because inline functions probably will increase binary size...


Btw, I have a suggestion for new swizzle functions. Speed should be around as fast as yours, but this is more user friendly and flexible, imo:

glm_swizzle4(v1, “wxyy”, v2);

this is interesting approach. I must learn how this (swz[2] - 'x') & 3 works. Maybe ASCII codes are consecutive / followed each other.

My goal was to use SIMD here but I couldn't :'(, that would be fastest one.

Ps: Your following defines are wrong, should be (3, 2, 1, 0), although you won’t need them much if you use my swizzle functions 😉 #define GLM_WZYX GLM_SHUFFLE4(0, 1, 2, 3) #define GLM_ZYX GLM_SHUFFLE3(0, 1, 2)

I think I have written tests for them. Nothing may be wrong, you may want to check GLM_SHUFFLE3 and GLM_SHUFFLE4 definitions.

recp avatar Apr 14 '20 14:04 recp