Remove transitive rendering includes from node.h#111405
Remove transitive rendering includes from node.h#111405Repiteo merged 1 commit intogodotengine:masterfrom
node.h#111405Conversation
Ivorforce
force-pushed
the
node-no-mesh
branch
from
f384858 to
9cc02a5
Compare
…des from `node.h`.
Ivorforce
force-pushed
the
node-no-mesh
branch
from
9cc02a5 to
33689d7
Compare
|
|
||
| #undef Window | ||
|
|
||
| class ArrayMesh; |
There was a problem hiding this comment.
Worth noting that this is the main fix, not changes to node.h.
Should scene_tree.cpp also have the STATIC_ASSERT_INCOMPLETE_TYPE calls?
There was a problem hiding this comment.
I considered it, but didn't add them since scene_tree.cpp isn't the "main include hub", but node.h is. So it's more important that node.h doesn't regress. But I'm happy to add it if you think we should.
There was a problem hiding this comment.
I guess it's fine as is, scene_tree.h doesn't seem to be included much explicitly outside node.h and main/platform entry points.
akien-mga
left a comment
akien-mga
left a comment
There was a problem hiding this comment.
That's a great improvement!
|
Thanks! |
|
This broke debug builds again 🙃 making a fix I think for any future reworks like they have to be built with debug builds to ensure they are actually not breaking them as we don't have debug templates break, unsure if it's 3D-disable related specifically (just testing one template) but we are getting at least one regression a week right now so some higher due diligence is really needed |
|
@AThousandShips Agreed, and sorry. I'm currently only making dev builds for these PRs. I'll start making debug and release builds for future include change PRs. |
|
Thank you! It seems to be specifically the 2D only build as some of the 3D includes have transitive includes It all works out, and I think I was just a bit frustrated at the lack of checks in CI getting in the way, but for major restructures it'd be appreciated testing some specific things to avoid errors As I build these regularly I can also see about building some things locally if people need and don't have things cached! |
4 participants
This PR reduces total compile time by 3% (going by a single test). Further, it will benefit rendering contributors because changes to rendering files will no longer cause a recompilation of almost all compile units.
Explanation
node.his a giant include hub. Most of our compile units include it in some way.It currently has an include path to rendering (
mesh.h,shader.h,rendering_server.h). However, mostNodeusers don't interact with rendering at all.This PR removes this include path (by removing the
mesh.hincludescene_tree.h), simply by forward-declaringArrayMesh.The cascading changes are individually simple and logical:
mesh.hin cpp files that have meshes of their own.Further, I'm making use of
STATIC_ASSERT_INCOMPLETE_TYPEto prevent regression of this PR.