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

Call API doesn't support macro definitions #454

Open
Lephar opened this issue Mar 16, 2025 · 4 comments
Open

Call API doesn't support macro definitions #454

Lephar opened this issue Mar 16, 2025 · 4 comments

Comments

@Lephar
Copy link

Lephar commented Mar 16, 2025

While it's possible to use CGLM like this:

#define CGLM_FORCE_DEPTH_ZERO_TO_ONE
#define CGLM_FORCE_LEFT_HANDED
#include "cglm/cglm.h"

the following code doesn't work as expected:

#define CGLM_FORCE_DEPTH_ZERO_TO_ONE
#define CGLM_FORCE_LEFT_HANDED
#include "cglm/call.h"

since the function bodies are already compiled.

It is possible to make it work by editting CMakeListst.txt/Makefile or with the tricks like C_FLAGS definitions while building but I've scanned the documentation/repository for clues and haven't found any official way to do it. If that's not the case please point me to the right direction, otherwise I am willing to contribute after a discussion about the proper way to implement it.

@recp
Copy link
Owner

recp commented Mar 19, 2025

Hi @Lephar

Thanks for reporting this,

Actually all RH and LH functions will be compiled into same binary. The problem is that for instance glmc_perspective() will pick default configuration which is glm_perspective_rh_no().

For now you can access LH/RH NO/ZO functions in full name like glm_perspective_lh_zo() or inline versions until this issue is solved.

  1. Adding flags to CMakeListst.txt/Makefile reflect the configuration seems right thing to do but I hope people wont mix compiled binaries if same naming is used. I mean one call glmc_perspective() could point glm_perspective_rh_no() while expecting glm_perspective_lh_zo()...
  2. Dropping functions like glmc_perspective() and force user to use full name in compiled version is an option too

Let’s get some feedback to get ideas on how to handle this

@Lephar
Copy link
Author

Lephar commented Mar 19, 2025

Thanks for replying!

Calling the RH/LH NO/ZO seems like the right choice for me right now. I can think of some disadvantages for both options:

  1. In case of compiling yourself, this seems right. But in case of precompiled binaries distributed by package managers, this would cause some confusion.
  2. This will be a breaking change for no purpose for users that are fine with the defaults.

So yeah, let's wait for more feedback.

@Lephar
Copy link
Author

Lephar commented Mar 20, 2025

Maybe we can inline default clipspace functions like glmc_perspective() even when using the Call API? It wouldn't make any difference in build and runtime performance since it is only a call to the respective function.

glmc_perspective(...) {
    #ifdef CGLM_FORCE_LEFT_HANDED
        #ifdef CGLM_FORCE_DEPTH_ZERO_TO_ONE
            glmc_perspective_lh_zo(...)
        #else
            glmc_perspective_lh_no(...)
        #endif
    #else
        ...
    #endif            
}

This approach doesn't cause neither of the problems I've mentioned but the default glmc_perspective() isn't really a "call" anymore.

@recp
Copy link
Owner

recp commented Mar 20, 2025

This is good approach, +1 for this.

The only problem may be direct replacement of new binary would cause linker error for dynamic lib, but still better and robust than existing solution.

Maybe we can get more feedback on a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants