Cannot build Godot with CSG module disabled

[Xrayez]

Godot version

3.4.beta

System information

Windows 10

Issue description

I've been working on updating CI configs in goostengine/goost#99, and stumbled upon link errors similar to:

Creating library bin\godot.windows.opt.64.goost.lib and object bin\godot.windows.opt.64.goost.exp
scene.windows.opt.64.goost.lib(room_manager.windows.opt.64.goost.obj) : error LNK2019: unresolved external symbol "public: class Array __cdecl CSGShape::get_meshes(void)const " (?get_meshes@CSGShape@@QEBA?AVArray@@XZ) referenced in function "private: bool __cdecl RoomManager::_bound_findpoints_geom_instance(class GeometryInstance *,class Vector<struct Vector3> &,class AABB &)" (?_bound_findpoints_geom_instance@RoomManager@@AEAA_NPEAVGeometryInstance@@AEAV?$Vector@UVector3@@@@AEAVAABB@@@Z)
bin\godot.windows.opt.64.goost.exe : fatal error LNK1120: 1 unresolved externals
scons: *** [bin\godot.windows.opt.64.goost.exe] Error 1120
scons: building terminated because of errors.

Use case: I disable all extra modules to speed up engine compilation via GitHub Actions.

Likely related to #46130.

Is CSG module required now? Is it possible to resolve this dependency? CC @lawnjelly.

Steps to reproduce

Compile the engine without CSG module enabled:

scons module_csg_enabled=no

Minimal reproduction project

N/A

[lawnjelly]

Ah I'll take a look and see. In order for rooms to work with CSG, it has to be able to get the meshes (i.e. the geometry). But I can probably make it so that when module_csg_enabled is off, it #ifdefs out this capability. :+1:

It looks like I might be able to implement the get_faces virtual function in CSGShape and use this as a means of getting geometry generically. I think this would be better.

[akien-mga]

With #50466 merged you can now fix it with:

diff --git a/scene/3d/room_manager.cpp b/scene/3d/room_manager.cpp
index 3f95d32c45..53bbbd86cf 100644
--- a/scene/3d/room_manager.cpp
+++ b/scene/3d/room_manager.cpp
@@ -36,13 +36,17 @@
 #include "core/os/os.h"
 #include "editor/editor_node.h"
 #include "mesh_instance.h"
-#include "modules/csg/csg_shape.h"
 #include "portal.h"
 #include "room_group.h"
 #include "scene/3d/camera.h"
 #include "scene/3d/light.h"
 #include "visibility_notifier.h"

+#include "modules/modules_enabled.gen.h"
+#ifdef MODULE_CSG_ENABLED
+#include "modules/csg/csg_shape.h"
+#endif
+
 // #define GODOT_PORTALS_USE_BULLET_CONVEX_HULL

 #ifdef GODOT_PORTALS_USE_BULLET_CONVEX_HULL
@@ -1412,6 +1416,7 @@ void RoomManager::_convert_portal(Room *p_room, Spatial *p_node, LocalVector<Por
 }

 bool RoomManager::_bound_findpoints_geom_instance(GeometryInstance *p_gi, Vector<Vector3> &r_room_pts, AABB &r_aabb) {
+#ifdef MODULE_CSG_ENABLED
    // max opposite extents .. note AABB storing size is rubbish in this aspect
    // it can fail once mesh min is larger than FLT_MAX / 2.
    r_aabb.position = Vector3(FLT_MAX / 2, FLT_MAX / 2, FLT_MAX / 2);
@@ -1459,6 +1464,9 @@ bool RoomManager::_bound_findpoints_geom_instance(GeometryInstance *p_gi, Vector
    } // if csg shape

    return true;
+#else
+   return false;
+#endif
 }

 bool RoomManager::_bound_findpoints_mesh_instance(MeshInstance *p_mi, Vector<Vector3> &r_room_pts, AABB &r_aabb) {

But if you find a solution that removes the module dependency that's even better of course.

[lawnjelly]

@akien-mga Ah that sounds fantastic. If you are happy to make a quick PR on that, it will work in the meantime.

My thoughts are that it would be better in the long run to have a generic solution for getting geometry from VisualInstances, via a virtual function, and that appears to be exactly what get_faces() is designed for. I need to double check this with reduz though, unfortunately there are no comments and @clayjohn tells me it may have originally been for the old lightmapper. It seems currently used by the ParticleEditor, NavMeshEditor for this kind of thing.

In a lot of ways we've been finding returning faces isn't ideal - @pouleyKetchoupp had a similar problem with convex hulls the other day. The API returns faces, which contains duplicate verts, where what we more often want is more akin to Geometry::MeshData (i.e. a list of vertices and indices, edges not so interesting).

This may have been driven by allowing the function to be accessible from gdscript, which can make it more difficult to return complex data, without making a structure to hold it.

The other problem I'm seeing with CSG is that get_faces() is const, and the CSG likes to build intermediate cached version of the geometry, which doesn't play well with the function being const. There may be an argument for making get_faces() non-const, but I'm really waiting to get some more info from reduz.

[akien-mga]

Sounds good, I'll make a PR. Might be worth opening a new issue or proposal for the geometry need you describe, it does sound like something worth addressing eventually which would benefit several areas.

[akien-mga]

Fixed by #50472.