Move server files into their subfolders#89409
Conversation
aaronfranke
force-pushed
the
server-folders
branch
2 times, most recently
from
21fd5f6 to
d13f9aa
Compare
|
This feels like a good call. But I don't about every server being in its own folder. Physic servers greatly benefit from it due of the other Godot Physics files, though. |
aaronfranke
force-pushed
the
server-folders
branch
from
d13f9aa to
56fe12d
Compare
There was a problem hiding this comment.
I support this, I've been meaning to propoose the same for a while. Our setup with the main server file in servers/ with its dependencies in servers/<type>/ always seemed a bit weird.
And now with the work done by @YeldhamDev to allow compiling without 2D/3D physics, 2D/3D navigation, XR, or RenderingDevice, this becomes more relevant.
One major argument against doing such change is that it breaks paths that C++ modules might rely on. But we're already breaking a lot of those paths with @YeldhamDev's PRs for 4.5 so we might as well do it all at once.
I'd like more @godotengine/maintainers opinions before we merge this though. @BastiaanOlij did raise a good point too.
|
Misread the git diff, ignore that. 😛 |
AThousandShips
left a comment
AThousandShips
left a comment
There was a problem hiding this comment.
The restructure for navigation looks good to me, didn't dig into the rest
servers/navigation_common.h
Outdated
| @@ -1,5 +1,5 @@ | |||
| /**************************************************************************/ | |||
| /* nav_heap.h */ | |||
| /* navigation_common.h */ | |||
There was a problem hiding this comment.
That sole navigation file under servers/ seems a bit weird.
We should consider if this alternative isn't better:
- Duplicate the shared enums in both the 2D and 3D folders, it's just a few constants and not uncommon in the Godot codebase to do this.
- Move the Heap class to
core.
There was a problem hiding this comment.
Another option is I could just move it into, say, the NavigationServer3D folder, and have this header be the only dependency 2D navigation has on 3D. Since it's not a cpp file, the build system wouldn't have to know. 🙈
There was a problem hiding this comment.
For now I'd say keep it as servers/nav_heap.h and move the enums back, duplicating them in 2D and 3D (maybe re-creating navigation_utilities_{2,3}d.h out of navigation_defaults_{2,3}d.h + these enums).
There was a problem hiding this comment.
I got sidetracked with scope creep so I opened PR #105718 which we can merge first.
There was a problem hiding this comment.
Another option is we could have servers/navigation/2d/ and servers/navigation/3d/ instead of servers/navigation_2d/ and servers/navigation_3d/, putting the common files in servers/navigation/. But then there is the question of if we would do this with physics as well.
KoBeWi
left a comment
KoBeWi
left a comment
There was a problem hiding this comment.
there was a easy to recognise difference between the outward facing servers, and their internal implementation.
I don't see much difference personally. But I also never had to include or touch server files 🤔
Server files follow a common structure: servers/topic/topic_server.h. The main file is still easy to identify. Alternatively we could rename them to the same name, but that's questionable.
Repiteo
left a comment
Repiteo
left a comment
There was a problem hiding this comment.
Invariably a very noisy change, but one that I think makes total sense with the added context of several other stylistic shuffling PRs similar to this one
|
Thanks! |
8 participants
This PR moves most of the
servers/*.{cpp,h}files into the subfolders, when they exist. The old approach always seemed off to me. If there is a folder for a thing, we should put all files for that thing in the folder.This PR also improves consistency, as some servers (debugger, extensions, and movie writer) did not have any top-level files.