-
Notifications
You must be signed in to change notification settings - Fork 32
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
GDExtension build prototype #294
base: master
Are you sure you want to change the base?
Conversation
Off to a great start! Do you have a good reason why restructure the directories into My intention with the current subdirectory structure is that Ideally, each one should have a separate CMakeLists.txt defining them as such, but that isn't the case due to laziness (low priority and not really required, until now?). I guess CMake namespaces can be used too (eg: Though I'd agree with separating out the OpenGL-dependent stuff. Maybe into
I figured. I remember reading about similar issues while reading about the planned "LibGodot" feature to allow using Godot as a library. Exceptions are enabled in OSP but we really don't use them (I personally don't like exceptions); it's possible to just disable them with a build flag. If |
It was mostly due to the opengl stuff. Once I figured I couldn't keep each directory as it was, I just split it in two. Your solution makes a lot more sense.
I'm not sure what causes the issue. std::call_once can raise an exception, but well. From what I've seen the Godot people say, the stdc++ library is not officially supported, and they make no effort to test if the features work, but if you give them a very specific and well-documented bug with it, they'll fix it. |
src/ospjolt/joltinteg.h
Outdated
BroadPhaseLayer GetBroadPhaseLayer(ObjectLayer inLayer) const override | ||
{ | ||
JPH_ASSERT(inLayer < Layers::NUM_LAYERS); | ||
return mObjectToBroadPhase[inLayer]; |
Check warning
Code scanning / PREfast
Reading invalid data from 'this->mObjectToBroadPhase'. Warning
[submodule "3rdparty/JoltPhysics"] | ||
path = 3rdparty/JoltPhysics | ||
url = https://github.com/jrouwe/JoltPhysics.git | ||
[submodule "3rdparty/godot-cpp"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably specify a branch (latest LTS? so 4.2 right now), the README for https://github.com/godotengine/godot-cpp?tab=readme-ov-file#versioning indicates tracking master is not a good idea unless you are building Godot from source to match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully up to date on how the .gitmodules
file works so maybe a new feature was added since i last looked, but as far as I understand, there isn't a way to explicitly indicate a branch in this file, and you have to instead rely on specifying the exact commit you want to refer to.
If they've added "Give me the latest in branch X" support, or "Give me whatever tag Y points to" support, i'd be happy to hear about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put the commit number, I'm pretty sure it's specified in the releases.
Can you make a PR that depends on your Jolt PR, that does nothing but purge newton? Not that said "remove newton" pr should be merged immediately, but the sooner we can see that it looks like the sooner we can get it done. |
I agree with @Capital-Asterisk that these should be kept as "stand-alone" libraries unless we run into something that can't be resolved without merging them. I also agree about the use of cmake namespaces. |
This seems like a fine suggestion to me. |
Well that's just shitty library/application design then. People can dislike exceptions all they want (C++'s implementation of exceptions has plenty of issues, obviously), but they exist and turning them off for such a large project is just stupid, as it leads to exactly the situation you point out with Though it's important to point out that That it's crashing for you (without an exception being thrown, even) strongly indicates that there is an implementation bug in the specific version of the standard library / compiler you're using. What standard library and compiler are you using? E.g. Operating system, compiler version, version of libstdc++ or libc++
It's true that we don't, but we've been pretty careful to go out of our way to design all of our code in a "can't fail" kind of way, with notable exceptions (heh) of math operations where we ignore floating point issues (though we don't currently bind signals to exceptions, so the program would just crash here), and "well that'll never happen" kinds of situations where the app'll break if the "never happen" thing happens. I think
function-static local variables are initialized with a two-phase atomic "is initialized?" check. The first phase is a non-atomic bool lookup (happy simple path, basically), the second is essentially an std::mutex. https://godbolt.org/z/8bMfoo36W
Ah, echo chambers. How lovely they are to live in. Do they have their fingers in their ears as well? |
I have no objection to this, but keep in mind that this does make it pretty difficult to pick up system libraries. Mileage may vary, since I've always wanted to make sure that 100% of our dependencies were always "in-tree", but others have been unhappy with that. |
@@ -222,30 +218,44 @@ ADD_SUBDIRECTORY(toml11 EXCLUDE_FROM_ALL) | |||
SET(SPDLOG_ENABLE_PCH ON CACHE BOOL "" FORCE) | |||
# Not yet supported by github runners | |||
# SET(SPDLOG_USE_STD_FORMAT ON CACHE BOOL "" FORCE) | |||
SET(SPDLOG_BUILD_PIC ON CACHE BOOL "" FORCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on this?
I don't mind it, but i'm curious to know your reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I had a link error due to positionning in spdlog, I saw there was a flag, I enabled it, it fixed the error.
Then again, I'm not sure how this all works, so if there is a preferred solution that's perfectly fine.
# SET(SKIP_INSTALL_ALL ON CACHE BOOL "" FORCE) | ||
# SET(SKIP_INSTALL_LIBRARIES ON CACHE BOOL "" FORCE) | ||
# SET(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE) | ||
# ADD_SUBDIRECTORY(freetype EXCLUDE_FROM_ALL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i recall correctly, we only need freetype for RmlUi.
So if we're going to go full steam on godot, then we can just drop RmlUi and freetype.
but in general, best not to leave commented out code in the source files.
|
||
SET(RML_SKIP_INSTALL ON CACHE BOOL "" FORCE) | ||
SET(CUSTOM_CONFIGURATION ON CACHE BOOL "" FORCE) | ||
#ADD_SUBDIRECTORY(RmlUi EXCLUDE_FROM_ALL) | ||
SET(ENABLE_ALL_WARNINGS OFF CACHE BOOL "" FORCE) | ||
SET(BUILD_SHARED_LIBS OFF CACHE BOOL "" FORCE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lovely, Jolt doesn't namespace it's options? Rather selfish of them.
CMakeLists.txt
Outdated
# Set project name | ||
PROJECT(OSP-MAGNUM CXX) | ||
|
||
# From: https://crascit.com/2016/04/09/using-ccache-with-cmake/ | ||
find_program( CCACHE_PROGRAM ccache ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally i don't think ccache specific things should live in the cmakelists.txt
In fact, this change will most likely screw up the ccache config that i (think...? did i? i guess i don't actually remember if i submitted it) setup in the continuous integration runners.
Instead, ccache support should live in a CMakePresets.json file
https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't know this was a thing, thank you. I just added it since it was in the template and the GdExtension takes a while to compile without it on my machine.
@@ -136,6 +162,8 @@ IF(MSVC) | |||
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/$<CONFIG> CACHE PATH "") | |||
# Force multiprocessor compilation for all projects | |||
add_compile_options($<$<CXX_COMPILER_ID:MSVC>:/MP>) | |||
# Force dynamic runtime library to be in debug mode in debug mode. | |||
add_compile_options($<$<CONFIG:Debug>:$<$<CXX_COMPILER_ID:MSVC>:/MDd>>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is already supposed to be the behavior that cmake has by default.
https://cmake.org/cmake/help/latest/variable/CMAKE_MSVC_RUNTIME_LIBRARY.html
If this variable is not set then the MSVC_RUNTIME_LIBRARY target property will not be set automatically. If that property is not set then CMake uses the default value MultiThreaded$<$CONFIG:Debug:Debug>DLL to select a MSVC runtime library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this is something for the Jolt PR, it was what was causing the windows compile error in debug mode in the CI. Adding that line fixed it. Maybe something in Jolt breaks it somehow ...
If that's the case, it might be better to upstream a fix in the long term.
CMakeLists.txt
Outdated
ADD_SUBDIRECTORY(test) | ||
|
||
IF (NOT OSP_BUILD_GDEXTENSION) | ||
ADD_SUBDIRECTORY(test) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a problem building unit tests when building a GDEXTENSION ?
src/CMakeLists.txt
Outdated
|
||
target_compile_features(osp-magnum PUBLIC cxx_std_20) | ||
#compile as position-independant to avoid link errors on static link | ||
add_compile_options("-fPIC") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target_sources(Adera PRIVATE "${CPP_FILES}" "${H_FILES}") | ||
|
||
# Enforce conformance mode for adera | ||
target_compile_options(Adera PRIVATE $<$<CXX_COMPILER_ID:MSVC>:/permissive->) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i recommend always leaving a trailing newline in files.
Otherwise unix based tools (and the github review viewer) give you angry red badges.
@@ -24,7 +24,7 @@ | |||
*/ | |||
#pragma once | |||
|
|||
#include <osp/drawing_gl/rendergl.h> | |||
#include <osp_drawing_gl/rendergl.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess i'm not following why you're changing the folder hierarchy like this?
is having a subdirectory causing a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly, I don't want to compile the drawing_gl part when building the GdExtension, to not depend on Magnum GL, SDL, and the like.
I figured it would be clearer to separate the common code, magnum-standalone-specific code, and Godot code in different directories instead of hiding it down the folder hierarchy. Also saves quite a few CMakeLists.txt files.
|
||
add_subdirectory( templates ) | ||
|
||
install( TARGETS osp-gdextension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the "install" step needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GdExtension have a required folder structure, as well as a template that needs to be copied in the right place, and the install step generates it properly.
Also, compiling with the cmake option to install the extension directly in you Godot project directory is super convenient imo. Godot reloads extensions immediately wh n the files change.
Since you re-arranged your commits for the jolt physics PR, can you rebase this PR to use the re-arranged commits as parent, instead of the original version? E.g. just rebase on master |
eb12d4c
to
49ee950
Compare
I made a new PR that removes newton, and rebased this PR on the previous one, as you had suggested. This should now be (mostly) clean. |
Small progress report: I got all the resources (gltf files and meshes) to load, via Magnum, in Godot. Basically the Turns out Godot doesn't really support std::cout, std::cerr, and the like. Which makes basically every logging function crash the program (and sometimes the engine). I kinda got around the issue by making Corrade logs go to stringstreams, that are then printed once per frame if they are not empty using the Godot print function. There might be a better way to do this for OSP logs, currently I just comment them when they cause an issue. My goal for now is really to get some examples working in the most ugly way, just to get an idea of what problems we might encounter with Godot. |
I honestly struggle with understanding how they could have fucked up |
Conflicts: src/adera_app/feature_interfaces.h src/adera_app/features/misc.h src/adera_app/features/newton.cpp src/adera_app/features/newton.h src/adera_app/features/physics.h src/adera_app/features/shapes.h src/adera_app/features/terrain.cpp src/adera_app/features/universe.h src/adera_app/features/vehicles.h src/adera_app/features/vehicles_machines.cpp src/adera_app/features/vehicles_machines.h src/adera_app/features/vehicles_prebuilt.cpp src/adera_app/features/vehicles_prebuilt.h src/osp/drawing_gl/rendergl.h src/osp_drawing_gl/rendergl.h src/planet-a/activescene/terrain.h src/testapp/MagnumApplication.cpp src/testapp/MagnumApplication.h src/testapp/MagnumWindowApp.cpp src/testapp/MagnumWindowApp.h src/testapp/features/magnum.h src/testapp/identifiers.h src/testapp/main.cpp src/testapp/scenarios.cpp src/testapp/scenarios.h src/testapp/sessions/common.cpp src/testapp/sessions/common.h src/testapp/sessions/jolt.h src/testapp/sessions/magnum.cpp src/testapp/sessions/magnum.h src/testapp/sessions/misc.cpp src/testapp/sessions/misc.h src/testapp/sessions/newton.cpp src/testapp/sessions/newton.h src/testapp/sessions/physics.cpp src/testapp/sessions/physics.h src/testapp/sessions/shapes.cpp src/testapp/sessions/shapes.h src/testapp/sessions/terrain.cpp src/testapp/sessions/terrain.h src/testapp/sessions/universe.cpp src/testapp/sessions/universe.h src/testapp/sessions/vehicles.h src/testapp/sessions/vehicles_machines.cpp src/testapp/sessions/vehicles_machines.h src/testapp/sessions/vehicles_prebuilt.cpp src/testapp/sessions/vehicles_prebuilt.h src/testapp/testapp.cpp src/testapp_magnum/MagnumApplication.cpp src/testapp_magnum/MagnumApplication.h src/testapp_magnum/main.cpp src/testapp_magnum/scenarios.cpp src/testapp_magnum/sessions/magnum.cpp src/testapp_magnum/sessions/magnum.h
I thought I sent this earlier? I already fixed all the things broken due to the framework changes. I have this branch that I just screw around in: Capital-Asterisk@1f52957 . This builds directly off of the gdextension-build branch, so you can just add my account as a git remote and |
I took your commits, added the aliasing fix, and axed all the useless rendering stuff (so now, osp is clearly just syncing it's entities with the rendered entities) Need to look into why it doesn't build on windows anymore though. I think we had the same problem before ? |
I think I'm mostly done (if the latest push passes CI) with what I want to include in this PR, so I'll mark it as ready. I cleaned up the code (which mostly means removed a lot of useless stuff here), addressed some very old comments, and solved all the errors and warning Godot was spouting out. The only issue left is the total lack of color and general rendering prettyness. But as this PR is starting to be quite big already, it might be wiser to add that in a secondary PR ? #300 should probably get merged before as this PR includes it. |
src/gdextension/render.cpp
Outdated
|
||
godot::SurfaceTool st; | ||
// why are there two different PrimitiveType enums ? | ||
st.begin(static_cast<godot::Mesh::PrimitiveType>(primitive)); | ||
|
||
godot::RID mesh = rs->mesh_create(); | ||
auto pos = meshData.positions3DAsArray(); | ||
// TODO copy other attributes as well maybe | ||
auto indices = meshData.indicesAsArray(); | ||
//TODO do that using an iterator I guess. | ||
uint32_t sg = 0; | ||
for ( auto i = indices.end(); i >= indices.begin() + 1;) | ||
{ | ||
st.set_smooth_group(sg++); | ||
auto v = pos[*(--i)]; | ||
st.add_vertex(godot::Vector3(v.x(), v.y(), v.z())); | ||
v = pos[*(--i)]; | ||
st.add_vertex(godot::Vector3(v.x(), v.y(), v.z())); | ||
v = pos[*(--i)]; | ||
st.add_vertex(godot::Vector3(v.x(), v.y(), v.z())); | ||
} | ||
if ( primitive == godot::RenderingServer::PRIMITIVE_TRIANGLES ) | ||
{ | ||
//st.deindex(); | ||
st.generate_normals(); | ||
//st.generate_tangents(); | ||
|
||
} | ||
|
||
godot::Array meshArray = st.commit_to_arrays(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the SurfaceTool that's leaking. Calling st.clear
at the end does not change anything, and from what I see, there is no other method which could help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume a RID is being leaked? though I notice there's no mention of RIDs in godot/scene/resources/surface_tool.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the leak: the way the indexed mesh is being turned into just vertices is throwing off the smooth shading. It looks like there's methods for directly adding an index buffer to SurfaceTool. I implied this somewhat in a previous comment, but it's likely that SurfaceTool isn't needed entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a RID, it's the object itself. There is apparently an "ObjectDB" in Godot, and it detects that all the surface tool used to create meshes are still allocated when the app closes.
I'll try tinkering it a bit, but I remember getting weird results when using other ways to create meshes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to the leak: the way the indexed mesh is being turned into just vertices is throwing off the smooth shading. It looks like there's methods for directly adding an index buffer to SurfaceTool. I implied this somewhat in a previous comment, but it's likely that SurfaceTool isn't needed entirely.
It turns out not ignoring normals fixes the smooth shading. Who could have thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as this PR is starting to be quite big already, it might be wiser to add that in a secondary PR
I think it's fair to get this merged quite soon, so others can start trying it out. Ideally, pull requests should be bite-sized; just getting a Godot window open would have been satisfactory.
Though honestly, I don't think we should care too much about master, since there's nothing at stake for screwing something up. IMO, these code reviews are best as learning oppurtunities more than anything in our case.
I can't seem to figure out how to get the framework changes out of the way to make this more readable; best solution I can find so far is using compare instead:
This is a heavy refactor of the project architecture and build commands to allow for the compilation of osp-magnum as a Godot GDExtension (see #289). It is very much a prototype intended for experimenting with Godot, not really a template of how things should be (names are whatever went through my head when I wrote them, some things are hardcoded...)
Currently I have not tested it, or even tried to make it work, for anything other than Linux x64.It is also based on my Jolt PR (#287 ), and I yeeted all the Newton stuff, as trying to get it to compile and link properly did not look pleasant.The unit tests were also giving me trouble so they're gone for now, I'll work on adding them back.(done)It adds a new
OSP_BUILD_GDEXTENSION
cmake flag, that when set, will compile osp as a shared library linked against godot-cpp, with (for the moment) the code from this GDExtension template repo as a "main", with an added call to ospjolt to check that it works. (About the license stuff, the template is Unlicense)For the project architecture, it splits the original code in two directories :common
andstandalone
.common
contains almost all the code from osp-magnum, with the exception of thetestapp
, and the opengl drawing stuff (adera/drawing_gl
, renamedshader
, andosp/drawing_gl
, renameddrawing_gl
). Standalone contains the rest.A
gdextension
directory was added, for the godot-specific code.The dependencies were modified. Mostly,common
depends on less features of Magnum, whilestandalone
depends on all the graphic stuff +common
. WhenOSP_BUILD_GDEXTENSION
is set, Magnum is compiled without all the graphics stuff and the main sdl app.I put a
CMakeLists.txt
in each subdirectory from the osp-magnum app, and additionally split the magnum-renderer specific code into additional directoriesosp_drawing_gl
,adera_drawing_gl
andtestapp_magnum
. Each directory only depends on what it needs to build.gdextension
additionally depends ongodot-cpp
, which was added to the project as a submodule in3rdparty
.Some libraries (Jolt and Freetype) were switched to static linking, as dynamic linking other libraires for a GDExtension is a bit of a pain.
As a side note, Godot seems to dislike (some parts of) the C++ standard library. Notably, it doesn't allow exceptions, which the standard library makes use of. It will probably lead to some issues (for example, calling
std::call_once
crashes the application). All the threading stuff may need to be implemented using Godot threads.