-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
TODO: Make cglm work with Metal, Vulkan and DirectX #152
Comments
I just ran into this issue over the weekend. I think providing secondary methods depending on the backend is a good idea. I am not a huge fan of how glm does it with a macro. |
@warmwaffles thanks for your feedbacks, sounds good to me ;) |
At risk of resurrecting an old thread -- for which apologies -- does the above relate to the way Vulkan requires its depth buffer, for example, to be zero-to-one and therefore the way that Any elaboration or discussion would be very much appreciated, as I'm learning all this as I go. Thanks. |
Hi @michael-g Thanks for your feedbacks,
+1 I'm interested to do some work on this, cglm could provide glm solution: template<typename T>
GLM_FUNC_QUALIFIER mat<4, 4, T, defaultp> perspective(T fovy, T aspect, T zNear, T zFar)
{
# if GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_LH_ZO
return perspectiveLH_ZO(fovy, aspect, zNear, zFar);
# elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_LH_NO
return perspectiveLH_NO(fovy, aspect, zNear, zFar);
# elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_ZO
return perspectiveRH_ZO(fovy, aspect, zNear, zFar);
# elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_NO
return perspectiveRH_NO(fovy, aspect, zNear, zFar);
# endif
} we can follow similar path. And yes we need update cglm for Vulkan clip space as soon as possible. Any help would be appreciated (Discussions, Feedbacks, PRs/Coding, testing and more testing...) I'have implemented If you or someone else could help to implement these stuff it would make us faster to add these features to cglm (putting resources here or to discussions maybe, help to coding, testing and more testing...) |
I would be happy to (try to) help, so long as we understand that any algorithms I implement are likely be a fairly slavish copy of what I see in GLM: I'm too much of a Luddite to extemporise those. I can of course avoid repeating calculations (in a similar way that |
Thanks 🤗
See: CONTRIBUTING.md |
Hi Recep, I've got myself a fork, a dev branch and a new function in
|
I'm not sure about the naming: glm_perspective_lh_zo();
glm_perspective_zo();
... vs glm_perspectiveLH_ZO();
glm_perspective_ZO();
...
You can check https://github.com/recp/cglm/blob/master/test/src/test_affine.h as test sample. More and complex tests would be awesome.
You can create a separate C++ project then test GLM and cglm both for compaing results maybe. Or you can add C++ file to cglm then remove it after if you are convinced that the results/implementations are correct. Also it is better to compare algorithm with original one on the paper, because GLM may have bugs...
Currently the tests cannot bee run on AppVeyor. Tests must be run manually on Windows by running test project, I coundn't rrun prrint the tests on AppVeyor. I'm using Mac, I can run tests, Travis also can run tests on macOS, Linux... https://travis-ci.org/github/recp/cglm . |
I have a working function with a unit-test now in this branch. I've also added a discrete/disconnected CMake C++ project stub under Please let me know your thoughts on this work-in-progress: it would be useful to find out if you're horrified by the changes before I make too many more. ;) EDIT: having done this I've realised I've been proceeding under a misapprehension, namely that Vulkan requires left hand NDCs (but does in fact require right hand NDCs). I'll correct the doc and amend the commit message. I think I was mislead by GLM (here)... |
@michael-g good work! I have some ideas which I want to share and get feedbacks; we can create separate files for each clip-space, for instance:
then we import all the headers into CGLM_INLINE
void
glm_perspective(float fovy, float aspect, float nearVal, float farVal, mat4 dest) {
/* GLM_CLIP_CONTROL_RH_NO may stay as default */
#if GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_LH_ZO
glm_perspective_lh_zo(fovy, aspect, nearVal, farVal, dest);
#elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_LH_NO
glm_perspective_lh_no(fovy, aspect, nearVal, farVal, dest);
#elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_ZO
glm_perspective_rh_zo(fovy, aspect, nearVal, farVal, dest);
/* #elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_NO */
#else
glm_perspective_rh_no(fovy, aspect, nearVal, farVal, dest);
#endif
} we can rename nearVal/farVal to zNear/zFar. Windows was failed to build cglm, because with separate files it would be easy to maintain and it may keep things clean. user can set We can provide convenience headers to make things easier for not-experts: default: #include <cglm/cglm.h>
/*
* Copyright (c), Recep Aslantas.
*
* MIT License (MIT), http://opensource.org/licenses/MIT
* Full license can be found in the LICENSE file
*/
#define GLM_CLIP_CONTROL_RH_ZO
#include <cglm/cglm.h> ... then if user want to onlly use Vulkan then he/she can do this by including /* use Vulkan clipspace and configuration */
#include <cglm/cglm-vulkan.h>
/* or Metal */
#include <cglm/cglm-metal.h>
/* or OpenGL */
#include <cglm/cglm-opengl.h>
/* or cglm default, user need set clipspace with macro,
if they don't want to use the default one exported by cglm */
#include <cglm/cglm.h> I'm working on a GPU library (https://github.com/recp/gpu) (in-progress) and Universal Shading Language (https://github.com/UniversalShading/spec) (in-progress) to make render layer GPU independent. Separating functions like typedef struct camera {
void (*perspective)(float, float, float, float, mat4);
/* ... */
} camera;
/* we can change clipspace just by changing pointer: */
camera->perspective = glm[c]_perspective_rh_no;
/* or */
camera->perspective = glm[c]_perspective_rh_zo; |
Excellent, I like the ideas. I'll make a start on implementing some of these and fix the It would be useful if you had any links to online resources that describe what these functions should all look like, since at the moment I'm relying on the GLM implementation, and what I can find online, e.g. this gem. It's way more detail than I need but at least it's clear how a Can I take it that you don't hate the |
Perfect 🤗 One resouce I suggest to look at is DirectXMath, AFAIK, they are implemented LH and RH functions. I'm not sure if they implemented Vulkan clipspace.
Let's implment
Sure. For Metal, I'll look at Apple docs later. I'll take a look API differences and may bring some resouces here. You can add anything you need to EDIT:
good resource. Maybe we could keep the resouce links in RESOURCES.md or in Wikis to take a look later if needed. CREDITS file already doing similar job but I'm nut sure, it's goal is different |
A few more changes in the branch. New |
Wow, now I undestand what you mean by It is better to avoid external deps. If we need tests to compare cglm with glm then we can create a separate repo for that.
I think postponing docs until coding has done is good idea since we may need to change somethings?
It looks good! It seems I didn't check if computations are correct or not, I'll try to do that later
+1
🤗 |
No problem, I can remove the subdirectory, Just to be clear, though: it was never part of the build. If you didn't know it existed you'd never need to link GLM into position. What I do think is important is that we have a way of regenerating the reference values I've used in the tests -- and to which I will add more functions as I reimplement/transpose/transliterate more of the L/RH Z/NO functions to cglm. If at some point in the future someone wants to validate where all those magic numbers came from, the content of |
I'm not sure really, let's keep folder clean, since we are not only trying to get GLM's results, separate-repo like
You are right, but what about separate test repo as I mentioned above? If someone want to validate cglm then he/she can clone test repo and run libary-library tests to compae cglm results with others e.g. GLM, Eigen... |
I hear you ;) In that case, the next steps seem something like
... and I'll remove the subdirectory under |
I've updated the branch and we should now have a complete set of LH/RH coord. system and ZO/NO clip-space variants. All have test coverage against both naive algo implemenations and magic numbers generated by the GLM equivalents. Happy either to raise a PR new for proper review or implement any other feedback you may have. |
Great! It would be better to continue with PR 🤗 We could get more reviews from other users maybe One thing I must say that be aware that |
Blimey... I missed this thread and started working on this myself! Good work though @michael-g ! Edit: Oh wait, how far have you got in implementation - I'm checking your branch out and perhaps we can work on this together if you're not completed. |
Where should |
@realaltffour thanks for help 🤗 I think working together will boost the process to implement the clip-space differences. What do you say @michael-g ?
I think each function must stay where it belongs. See #152 (comment) comment. |
Great, I think having them in |
+1
I didn't check yet, if so one can be another's wapper instead of duplcating implementations. |
@michael-g ping. |
Thanks for these, I'll have a look.
Re the above, while I love the explanations and the way it's all laid out for us, I can't reconcile his final C++ code with any of the [RL]H_[NZ]O implementations in GLM. In particular, his value for |
Hi @raedwulf, sorry, been busy! The branch is up-to-date with my local changes and I was going to do some more work on it tonight. However, I'm happy to be hands-off until you've added your implementations to the branch. Did you really implement all four variants for all six ortho functions? It's quite an explosion of function variants. Excellent work if you did! One happy aspect is that I don't think we need to add any more files, which is a bit painful because of the number of places you need to remember to add them (both @recp, re:
there is one material difference in the
while my implementation, which oddly agrees with the GLM implementation has
The other difference should -- I think -- be non-material, as the existing impl is Any thoughts on the divergence between the existing cglm implementation and the GLM equivalent? Have I just written it down wrong? |
But let's grow it together 👶
Yes moving these rh/lh/no/zo headers to cam.h: ...
#ifndef cglm_vcam_h
#define cglm_vcam_h
#include "common.h"
#include "plane.h"
/* or */
#include "clipspace/cam.lh.no.h"
#include "clipspace/cam.lh.zo.h"
#include "clipspace/cam.rh.no.h"
#include "clipspace/cam.rh.zo.h"
/* or */
#include "clipspace/cam-lh-no.h"
#include "clipspace/cam-lh-zo.h"
#include "clipspace/cam-rh-no.h"
#include "clipspace/cam-rh-zo.h"
/* or */
#include "clipspace/cam_lh_no.h"
#include "clipspace/cam_lh_zo.h"
#include "clipspace/cam_rh_no.h"
#include "clipspace/cam_rh_zo.h"
... we can include related files conditionally if we want: ...
#ifndef cglm_vcam_h
#define cglm_vcam_h
#include "common.h"
#include "plane.h"
#ifndef CGLM_CLIPSPACE_INLUCE_ALL
# if GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_ZO
# include "clipspace/cam_rh_zo.h"
# elif GLM_CONFIG_CLIP_CONTROL == GLM_CLIP_CONTROL_RH_NO
# include "clipspace/cam_rh_no.h"
# elif ...
...
# endif
#else
# include "clipspace/cam_lh_no.h"
# include "clipspace/cam_lh_zo.h"
# include "clipspace/cam_rh_no.h"
# include "clipspace/cam_rh_zo.h"
#endif
Here is the documentation I were followed: it is exactly same as The GLM's implementation seems different: I'm not sure why it is different maybe it fixes something e.g. improves projection precision maybe? |
@michael-g Yeah I'll do the move to clipspace. I have a couple of files left to do as I originally implemented it all in cam.h and merged the decomp functions. With the |
I vote for /* from (existing impl) */
dest[1][0] =-(e * t[0] - g * t[3] + h * t[4]);
/* to (new impl) */
dest[1][0] = g * t[3] - e * t[0] - h * t[4]; I'll push to a PR after finished. Hope it will worth it. Negating SIMD register requires extra instruction[s] (and exta load on x86) EDIT: I don't know if there is any difference in FPU/SIMD for |
@raedwulf you're a gent, that would be great. FWIW,
feels the most "C like" to me.
Speaking from a position of ignorance, my guess is that it flips the one of the axes to avoid everyone having to implement some hack or other. But it's a pure guess. Are we going to implement the coordinate system/clip-space implementation-hiding like GLM does here? I'm not sufficiently experienced in 3d programming to know whether the provision of "partially hidden" functions has any utility, e.g. providing @recp, I have no idea what I was thinking when I wrote
I must be delirious from lack of sleep. What I wrote down was |
I'll investigate this or I may ask to author of GLM later. What kind of hack may I ask? |
I'll have some time to work on it today (I just started a new job yesterday so was a bit exhausted)! |
https://github.com/raedwulf/cglm/tree/clipspace-wip I've not done the code shifting or tested it (I'm sure I've missed some things), but I'll have more time today when I get home! |
@raedwulf many thanks 🤗 I'll take a look asap |
@raedwulf you're a machine ;) awesome work. Let me know if there's a discrete area in which I can contribute to the tests etc. I'm not sure how much time I'll have but just in case... |
Thanks! Yeah tests would be great :) I'll still need to rearrange the files a bit - hopefully will have some time later today. |
Update - will have a chance this weekend to work more on this, but a bit exhausted last few days. @michael-g no hurry! |
Roger that. Take it easy! |
I'm still waiting a PR ;) Let's discuss changes on the PR? I'll check your branch soon, Let's make cglm work with LH/RH and NO/ZO 🤗 |
Perspective function from Apple's Metal sample: matrix_float4x4 matrix_perspective_right_hand(float fovyRadians, float aspect, float nearZ, float farZ)
{
float ys = 1 / tanf(fovyRadians * 0.5);
float xs = ys / aspect;
float zs = farZ / (nearZ - farZ);
return (matrix_float4x4) {{
{ xs, 0, 0, 0 },
{ 0, ys, 0, 0 },
{ 0, 0, zs, -1 },
{ 0, 0, nearZ * zs, 0 }
}};
} Also I liked |
Thanks, I've updated my branch with the new nearZ/farZ names - will push me stuff later after I check it compiles. |
@realaltffour awesome 🤗 |
@raedwulf ping |
Just working on compilation errors :) |
:) |
Awesome. |
Follow at #198 Thanks @raedwulf, @michael-g |
cglm must support Metal and Vulkan (and maybe DirectX). To do this cglm may provide alternative functions for alternative NDC coordinates. Or we could do that with preprocessor macros. But providing extra functions will provide ability to switch between graphics APIs without rebuilding the code.
Resources:
The text was updated successfully, but these errors were encountered: