Polytonic / Glitter

Dead Simple OpenGL
http://polytonic.github.io/Glitter/
2.48k stars 418 forks source link

Switched GLFW with SDL2, Removed Bullet #19

Closed zach2good closed 9 years ago

zach2good commented 9 years ago

Don't know if this is of any use for you to have as an example, but I swapped GLFW for SDL2 in this branch. Your CMake script really helped me a lot!

Polytonic commented 9 years ago

Hi there! I'm glad you found Glitter useful! Thanks for your pull request, but I'm going to have to decline this one, sorry!

GLFW was an explicit choice over SDL, for a number of reasons, but foremost: both linked tutorials use GLFW over SDL. I have a few other reasons, but they're not really worth rabbit-holing on right now.

Just some comments for you (in general):

+[submodule "Glitter/Vendor/sdl"]
+   path = Glitter/Vendor/sdl
+   url = https://github.com/zach2good/SDL.git

It is unsafe for Glitter to rely on a version of SDL that you are self-hosting. I wouldn't want to keep asking you to update your version so that I can update the submodules here. This should really be pointing to the official SDL repo (possibly bridged by git-hg), or a trustworthy git mirror.

+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic -std=c++14")

Specifically, C++14 here, a fair number of Glitter users are running on compilers that may not support the C++14 flag yet. When submitting a pull request to Glitter, please try to only include the minimum needed to get your contribution working, without "upgrades" that are personal choices. Toolchain and infrastructure upgrades in C++ are a nontrivial headache, especially on large teams.

+//#if defined(_MSC_VER) && !defined(_CRT_SECURE_NO_WARNINGS)
+//#define _CRT_SECURE_NO_WARNINGS
+//#endif

Please do not commit commented-out code, especially ones that include preprocessor directives. This is what version control is for. :smile:

Oh, and don't delete Bullet and pull-request said deletion into the main repository, especially when the previous request was to add it. If you don't think it belongs in Glitter, feel free to open an issue and we can discuss it.

Thanks again!