Makefile makeover - Githubissues

gruelingpine185 commented 1 year ago

I know that @tulpenkiste is working on a PR related to #1, but I made this PR to provide support for MacOS, Linux, and Windows in addition to what they done. Here are the things this PR takes care of:

Building static and dynamic libraries
Building Glad (given that its source and headers are placed in libs/)
Building the cbengine executable
Compilation on MacOS (tested), but hopefully Windows, and Linux (based on what's already in the current Makefile and what GLFW's build guide says). Support can easily be added
Resolve compilation warnings for:
- Different sized integer comparisons
- Comparison of address to NULL
- Closes #1, (maybe #3)

tulpenkiste commented 1 year ago

Tried this, it doesn't use my system's GLFW headers (in /usr/include/GLFW) which is a tad annoying

gruelingpine185 commented 1 year ago

I have ifdef () guards for mac, linux, and windows. Instead of $(shell pkg-config ...) try placing your system GLFW path.

tulpenkiste commented 1 year ago

Nevermind, it now found my system headers but it no longer links against the system GLFW install. (uses -lglfw3, should use -lglfw)

tulpenkiste commented 1 year ago

Additionally, line 64 of the makefile is unneeded as the compiler already does OS specific defines.

__APPLE__
__WIN32
__linux__

gruelingpine185 commented 1 year ago

True. If Ceebox is learning C++, I'd rather have him be able to easily add platform specific stuff by using colloquial names like MACOS or WINDOWS or LINUX rather than the predefined defines. For example __APPLE__ isn't the only MacOS define, same thing for windows and Linux. Here is a list of OS defines to help prove my point. Different macros work on different platforms. To circumvent that, by getting the overarching OS in the Makefile and creating a define based on that OS, there is no longer a need to distinguish between which defines should be used for which specific platform...

tulpenkiste commented 1 year ago

I do think it would be better to use the actual OS defines rather than just doing -D$(PLATFORM)=1

gruelingpine185 commented 1 year ago

I'll add them and fix a potential bug on line 66

gruelingpine185 commented 1 year ago

I'm working a complete rewrite of all the code in this repository. Moving code into namespaces, classes, and what not. I'm striving to make a clean codebase that will allow for simple modifications, a nice API, etc. in the future. It'll be based on this PR. While I'm here, can someone explain what renderer.cpp actually does? I made a class for windows, a basic main loop, and stuff for all the other files already. The following is a snippet of Renderer::Init() that I don't understand what to do with.

https://github.com/Ceebox/cbEngine/blob/a47b4aa2e4a366a946a1c534e83da95cb93eea9b/src/renderer.cpp#L37-L153

What does the snippet above do?
How can/should I create function/s that does the above?
And if I can't or shouldn't make a function for it, what do I do with it?

unendlicherPing commented 1 year ago

I'm working a complete rewrite of all the code in this repository. Moving code into namespaces, classes, and what not. I'm striving to make a clean codebase that will allow for simple modifications, a nice API, etc. in the future. It'll be based on this PR. While I'm here, can someone explain what renderer.cpp actually does? I made a class for windows, a basic main loop, and stuff for all the other files already. The following is a snippet of Renderer::Init() that I don't understand what to do with.

https://github.com/Ceebox/cbEngine/blob/a47b4aa2e4a366a946a1c534e83da95cb93eea9b/src/renderer.cpp#L37-L153

What does the snippet above do?

How can/should I create function/s that does the above?

And if I can't or shouldn't make a function for it, what do I do with it?

From line 37 to line 60 the shader code is defined. Usually the shader code is loaded from a file to provide an easy way to change the shader code without recompiling the c code. That is not needed with the current approach of just rendering one texture to the screen but could be useful in the future. For now the strings can be kept as constants.

From line 62 to 89 the shaders are getting compiled into a shader program and is stored in the shaderProgram variable. And in line 167 the shaderProgram gets applied to the future draw calls. I would think of an API somewhat like this:

struct ShaderProgram {
  GLuint shaderProgram; // Same as unsigned int but GLuint is the official return type of glCreateProgram and glCreateShader
}

ShaderProgram *create_program(char *vertex_shader_source, char *fragment_shader_source);
void           use_program(ShaderProgram *program);

I never used c++ and only used c but it should be easy to do it in a more OOP style.

That's all for the shaders.

unendlicherPing commented 1 year ago

I'm working a complete rewrite of all the code in this repository. Moving code into namespaces, classes, and what not. I'm striving to make a clean codebase that will allow for simple modifications, a nice API, etc. in the future. It'll be based on this PR. While I'm here, can someone explain what renderer.cpp actually does? I made a class for windows, a basic main loop, and stuff for all the other files already. The following is a snippet of Renderer::Init() that I don't understand what to do with.

https://github.com/Ceebox/cbEngine/blob/a47b4aa2e4a366a946a1c534e83da95cb93eea9b/src/renderer.cpp#L37-L153

What does the snippet above do?

How can/should I create function/s that does the above?

And if I can't or shouldn't make a function for it, what do I do with it?

Line 91 to 94 is not doing anything and can just be removed like its done in one of the currently open prs.

The next part defines some Vertrcies with attributes and tells OpenGL how to read those attributes. I have no idea how to abstract this because this can differ very much for different objects to render. I would just throw it all in a method and return the vao but I don't know how to work with the deallocation of the vbo and ebo variables. This could be solved with creating a class with the member variables ebo vbo and vao which initializes with the code from line 96 to line 132. Destructs with the code from line 182 to line 185(Same for the shader program class: line 186). And has a method which executes the code on line 173.

To make the vertecies definition on line 97 a little bit more clean I would create a Vertex struct like this.

struct Vertex {
  float pos_x, pos_y;
  float tex_x, tex_y; // should be named better
}
// I have an open pr that would remove all mentions of color and z attribute because they are not needed

Vertex vertecies[] = ...

gruelingpine185 commented 1 year ago

Thanks so much for your help. I'll have a branch with my progress so far up maybe sometime soon. Oh, and would it be easier for you if I just convert all the code to C and make #ifdef __cplusplus guards around it for those who use C++? I guess it's a question for @Ceebox considering he was moving to C++ from C.

Ceebox / cbEngine

Makefile makeover #6