dedicated_server feature tag does not enable GODOT_SERVER preprocessor define

warent commented 1 month ago

Tested versions

Godot v4.3.beta3.mono

System information

Godot v4.3.beta3.mono - Windows 10.0.22631 - Vulkan (Mobile) - dedicated NVIDIA GeForce RTX 3050 Ti Laptop GPU (NVIDIA; 31.0.15.4680) - 12th Gen Intel(R) Core(TM) i7-12650H (16 Threads)

Issue description

When running an instance with the dedicated_server feature tag, it runs headless as expected. However, the C# GODOT_SERVER preprocessor define does not work. I would expect that to be enabled.

Steps to reproduce

Create C# project
Configure run instance with dedicated_server feature tag
Run some code with #if GODOT_SERVER
Observe that code is never reached

Minimal reproduction project (MRP)

preprocessorrepro.zip

warent commented 1 month ago

I've been looking into the engine and think I have a better understanding of the problem now.

There is no such GODOT_SERVER preprocessor define, so the docs here seem to be incorrect: https://docs.godotengine.org/en/stable/tutorials/scripting/c_sharp/c_sharp_features.html#full-list-of-defines
Feature tags like dedicated_server are only passed into the instance after build (via OS environment GODOT_EDITOR_CUSTOM_FEATURES) so there can be no GODOT_SERVER define based on these tags unless we hoist the property up into the Mono BuildManager.cs chain.
Furthermore, in order to support a GODOT_SERVER preprocessor define, we would have to compile a separate executable for every run instance (or at least a special server executible which has this preprocessor define) which is currently not done.

I'd be happy to help with trying to apply a patch for this, but given the above, I think it requires architectural decisioning. Let me know what y'all think!

warent commented 1 month ago

My recommendation is that we detect if there is a run instance which specifies dedicated_server, and if so then run a separate build for it when clicking run. That might slow things down a bit so maybe this feature could be marked as optional?

warent commented 1 month ago

@AThousandShips I'm not sure if anyone will see my comments above so just tagging someone here to make sure it's on the Godot team's radar!

AThousandShips commented 1 month ago

Please don't tag random people, it helps no one, I'll tag this but it would have been processed regardless

warent commented 1 month ago

Please don't tag random people, it helps no one

Hmm not to be difficult, I don't think that's mentioned in the contributor guidelines, which is confusing to me. (Also for what it's worth, I didn't consider you a "random person" here)

My communication style is very different. I always appreciate a ping (at work or outside!) because I can just look at it later at my earliest convenience. So, to me, it's just a good measure of bridging communication (overcommunication is better than gaps).

Now I understand it looks like that's not how things are done here, which is okay, it's just not really clear or obvious to me. It's very possible I just missed it though if it is mentioned or clarified somewhere. Otherwise maybe I can create a ticket suggesting updating communication guidelines somewhere?

I'll tag this

Thank you for that and the quick reply!

AThousandShips commented 1 month ago

I wasn't admonishing or criticizing, just advising, and that issues will get checked out as soon as they can, this was opened less than a day ago so there's no rush. And people already have things on their plate and random pings aren't always useful, that's all my point is. This isn't a rule or anything, just an observation on the process of triage.

So no worries, just stating my point and some constructive feedback on the process from someone who has experience with the triage process, and the scale of things to look into and the amount of time we have to engage with

By random I meant someone who:

Isn't relevant to the topic specifically (i.e. I'm not a dotnet developer)
Hasn't engaged with the issue already
Hasn't talked about it on other channels
Etc

warent commented 1 month ago

Understood! Thank you for the feedback and your help 🙏

AThousandShips commented 1 month ago

It's unclear if the documentation is at fault here, it doesn't mention the dedicated_server feature tag, nor does the page for export feature tags make mention of dedicated_server either, I suspect the GODOT_SERVER is a leftover from the 3.x days when there was a server build

This should probably help with solving this problem though, if this isn't decided to be an issue but something to update in the documentation:

https://github.com/godotengine/godot/pull/93539

Edit: Searching through the 4.x codebase there's not a single reference to GODOT_SERVER so I think the mention in the docs is a leftover from 3.x and doesn't apply at all to 4.x, hence why it isn't mentioned anywhere else

raulsntos commented 1 month ago

Regarding the GODOT_SERVER preprocessor define, AThousandShips is correct. Server was a platform in Godot 3.x but was removed in 4.0 (see https://github.com/godotengine/godot/pull/64846 for the removal of the platform from the .NET module). So the documentation is outdated.

Since you also mention feature tags. Rather than using #if, you can check for feature tags using the OS.HasFeature API.

aldocd4 commented 1 month ago

Regarding the GODOT_SERVER preprocessor define, AThousandShips is correct. Server was a platform in Godot 3.x but was removed in 4.0 (see #64846 for the removal of the platform from the .NET module). So the documentation is outdated.

Since you also mention feature tags. Rather than using #if, you can check for feature tags using the OS.HasFeature API.

Hello, I have the same issue here, this solution is not viable. I will make it simple: I'm building a MMORPG using Godot RPC, and I don't want to share the server-side code. So I have #if GODOT_SERVER || TOOLS || DEBUG on almost all my server-side files and RPC functions.

When exporting a build, the assembly is built using one of the VS configuration generated by godot (Debug, ExportDebug, ExportRelease). You can't add a custom preprocessor in Godot (https://github.com/godotengine/godot/blob/e684d126ed605899267ee37cc143f072b93b4d04/modules/mono/godotsharp_dirs.cpp#L58).

The solution I use right now is to configure the C# project manually before exporting the server build.

It works but it's not safe (you can build the client with that preprocessor by mistake) and you have to change it everytime you export the client/server. I think that we should have an easier way to add this preprocessor on godot side when exporting (I will soon build the server/client builds using a CI for example, I don't know yet how I'm gonna do that).

AThousandShips commented 1 month ago

I think we need to separate the question of allowing a feature like this (which in an enhancement, covered by https://github.com/godotengine/godot-proposals/issues/10028) and the now obsolete and completely unrelated GODOT_SERVER define that has nothing to do with the dedicated server mode as explained above

So this issue is about how the documentation is outdated, not about the broader feature, that's covered by that proposal

godotengine / godot