Skip to content
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

Re-add project features as define constants in C# #53920

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Oct 17, 2021

Adds the project feature tags as C# define constants.

This includes custom features, support for this was added in #28786 but got removed in #40595 and #41408 on the move to Godot.NET.Sdk.


For a project exported with this configuration:

image

The C# project will have the following constants defined:

  • GODOT_FEATURE_64
  • GODOT_FEATURE_X11
  • GODOT_FEATURE_MY_GREAT_FEATURE
  • GODOT_FEATURE_PC
  • GODOT_FEATURE_S3TC

Then in the code you can use it like so:

#if GODOT_FEATURE_MY_GREAT_FEATURE
	GD.Print("my_great_feature supported");
#endif

Comment on lines +189 to +356
string sanitizedFeature = feature.ToUpperInvariant()
.Replace("-", "_")
.Replace(" ", "_")
.Replace(";", "_");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be too naive, it follows the same rules that were implemented by #28786

@@ -170,9 +171,32 @@ public static bool BuildProjectBlocking(string config, [CanBeNull] string[] targ
if (Internal.GodotIsRealTDouble())
buildInfo.CustomProperties.Add("GodotRealTIsDouble=true");

if (features != null && features.Length > 0)
buildInfo.CustomProperties.Add($"GodotFeatures={string.Join("%3B", SanitizeFeatures(features))}");
Copy link
Member Author

@raulsntos raulsntos Oct 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%3B is ; escaped, I couldn't get it to work otherwise

@Chaosus Chaosus added this to the 4.0 milestone Oct 17, 2021
@Chaosus Chaosus requested a review from neikeq October 17, 2021 15:07
@neikeq
Copy link
Contributor

neikeq commented Oct 17, 2021

The reason for the removal of the engine feature constants is as I commented in #40595:

Godot defines a list of constants for conditional compilation. When exporting games, this list also included engine 'features' and platform 'bits'. There were a few problems with that:

  • The 'features' constants were only defined when exporting games. Not when building the game for running in the editor player.

  • If the project was built externally by an IDE, the constants wouldn't be defined at all.

The new Sdk assigns default values to these constants when not built from the Godot editor, i.e.: when built from an IDE or from the command line. The default define constants are determined from the system MSBuild is running on.

However, it's not possible for MSBuild to determine the set of supported engine features. It's also not possible to determine if a project is being built to run on a 32-bit or 64-bit Godot executable.

As such the 'features' and 'bits' constants had to be removed. The benefit of checking those at compile time was questionable, and they can still be checked at runtime.

These problems are still present in this new PR and I don't think there's a way to solve them hence why I removed them.

@raulsntos
Copy link
Member Author

raulsntos commented Oct 17, 2021

The 'features' constants were only defined when exporting games. Not when building the game for running in the editor player.

Yeah, but that's how feature tags work. The method call to OS.HasFeature("my_feature") in runtime only returns true in exported games that were exported with the feature. I don't think there is a way to run the game from the editor with feature flags defined (C# or GDScript)1.

If the project was built externally by an IDE, the constants wouldn't be defined at all.

Most IDEs should allow you to specify your own arguments for the build command so you could add the constants there the same way this PR adds them to the msbuild command.

msbuild /p:GodotFeatures=GODOT_FEATURE_EXAMPLE

Or you could temporarily add it to the .csproj:

<GodotFeatures>GODOT_FEATURE_EXAMPLE</GodotFeatures>

These problems are still present in this new PR and I don't think there's a way to solve them hence why I removed them.

I agree, I don't think there's a way to solve it but I don't think it needs to be solved. We could consider this an advanced feature, most people can use runtime checks, but for users that know how this works they can take advantage of it so I think it's still worth having even with those limitations.

One example I just thought of that having these constants defined allows would be including a nuget package conditionally like so:

<Project Sdk="Godot.NET.Sdk/3.4.0">
  <PropertyGroup>
    <TargetFramework>netstandard2.1</TargetFramework>
  </PropertyGroup>
  <ItemGroup Condition=" $(GodotFeatures.Contains('GODOT_FEATURE_THAT_USES_JSON')) ">
    <PackageReference Include="System.Text.Json" Version="5.0.0" />
  </ItemGroup>
</Project>

Footnotes

  1. Actually, there's a proposal about setting feature flags for running the game from the editor (https://github.com/godotengine/godot-proposals/issues/373).

@akien-mga
Copy link
Member

@neikeq Would be nice to have this reviewed :) (also the 3.x counterpart)

@DmitriySalnikov
Copy link
Contributor

I recently added the ability to generate a C# API for my GDExtension library, and since it is intended for debugging, I would not like it to work in the release build. But some users would like to have debug rendering in the release build as well. To do this, I use the feature tag, which is currently not taken into account by the C# build system.
Therefore, users will have to manually add the preprocessor flag.

Will this PR be merged?

@raulsntos
Copy link
Member Author

raulsntos commented Sep 15, 2023

@DmitriySalnikov You should be able to use the same API that is available in GDScript to check if a feature tag is enabled: OS.HasFeature("forced_dd3d"). So you could do something like this:

bool isDebug =
#if DEBUG
	true;
#else
	false;
#endif

if (isDebug || OS.HasFeature("forced_dd3d"))
{
	// Use the normal library.
}
else
{
	// Use the dummy library.
}

@DmitriySalnikov
Copy link
Contributor

DmitriySalnikov commented Sep 15, 2023

@ DimitriPilot3

wrong mention

I would like users to lose as little performance as possible from calling such methods. Because this API allows users not to delete debugging methods in the release build, both in GDScript and in C#.

    public static void DrawSphereXf(Transform3D transform, Color? color = null, float duration = 0f)
    {
#if DEBUG || FORCED_DD3D
        Instance?.Call(__draw_sphere_xf, transform, color ?? _DebugDrawUtils_.DefaultArgumentsData.arg_0, duration);
#endif
    }
    
    public static void DrawSphereHd(Vector3 position, float radius = 0.5f, Color? color = null, float duration = 0f)
    {
#if DEBUG || FORCED_DD3D
        Instance?.Call(__draw_sphere_hd, position, radius, color ?? _DebugDrawUtils_.DefaultArgumentsData.arg_0, duration);
#endif
    } 

@raulsntos
Copy link
Member Author

wrong mention

Sorry about that.

I would like users to lose as little performance as possible from calling such methods.

I'm not sure the performance cost is significant enough to be noticeable, but I haven't benchmarked it. The biggest cost is probably the interop call to OS.HasFeature, but you should also be able to cache the returned value since I don't think features can change during runtime.

	private bool _isDebug =
#if DEBUG
		true;
#else
		false;
#endif

	private bool? _cachedForcedDD3D;
	private bool ForcedDD3D => _cachedForcedDD3D ??= OS.HasFeature("forced_dd3d");

	public static void DrawSphereXf(Transform3D transform, Color? color = null, float duration = 0f)
	{
		if (_isDebug || ForcedDD3D)
		{
			Instance?.Call(__draw_sphere_xf, transform, color ?? _DebugDrawUtils_.DefaultArgumentsData.arg_0, duration);
		}
	}

I do think having the feature tags as define constants in C# would be nice, and that's why I created this PR. I can't guarantee that this PR will ever get merged, so in the meantime, I hope this workaround can be useful to you.

@DmitriySalnikov
Copy link
Contributor

DmitriySalnikov commented Sep 15, 2023

In this form, I can try to use it. Thanks for the idea.

Unfortunately, all this code will still be compiled into one assembly, but it is unlikely to make any difference.


upd. Runtime check is 2-3 times slower when DEBUG is disabled.

Methods:

public static void DrawBoxXf(Transform3D transform, Color? color = null, bool is_box_centered = true, float duration = 0f)
{
    if (_DebugDrawUtils_.IsCallEnabled) // Equal to DEBUG
    {
        Instance?.Call(__draw_box_xf, transform, color ?? _DebugDrawUtils_.DefaultArgumentsData.arg_0, is_box_centered, duration);
    }
}

public static void DrawBoxXfDEF(Transform3D transform, Color? color = null, bool is_box_centered = true, float duration = 0f)
{
#if DEBUG
    Instance?.Call(__draw_box_xf, transform, color ?? _DebugDrawUtils_.DefaultArgumentsData.arg_0, is_box_centered, duration);
#endif
}

Benchmark:

int iters = 100000;
GD.Print($"Iterations = {iters}");
GD.Print($"Is enabled? {_DebugDrawUtils_.IsCallEnabled}");
var time = Time.GetTicksUsec();

for (int i = 0; i < iters; i++)
{
    DebugDraw3D.DrawBoxXf(Transform3D.Identity);
}
GD.Print($"Call IF: {((Time.GetTicksUsec() - time) / 1000.0):F3} ms");
time = Time.GetTicksUsec();

for (int i = 0; i < iters; i++)
{
    DebugDraw3D.DrawBoxXfDEF(Transform3D.Identity);
}
GD.Print($"Call DEF: {((Time.GetTicksUsec() - time) / 1000.0):F3} ms");

Results (Optimize = on):

Iterations = 10000
Is enabled? False
Call IF: 0,081 ms
Call DEF: 0,046 ms

Iterations = 10000
Is enabled? False
Call IF: 0,083 ms
Call DEF: 0,045 ms

Iterations = 100000
Is enabled? False
Call IF: 1,246 ms
Call DEF: 0,496 ms

Iterations = 100000
Is enabled? False
Call IF: 1,203 ms
Call DEF: 0,500 ms

Iterations = 10000
Is enabled? True
Call IF: 63,877 ms
Call DEF: 47,085 ms

Iterations = 10000
Is enabled? True
Call IF: 46,609 ms
Call DEF: 60,907 ms

Iterations = 100000
Is enabled? True
Call IF: 545,362 ms
Call DEF: 522,706 ms

Iterations = 100000
Is enabled? True
Call IF: 544,562 ms
Call DEF: 572,456 ms

@FreshDaikon
Copy link

FreshDaikon commented Feb 18, 2024

@raulsntos / @neikeq - Does something additional happen to be injected/build into the pipeline when exporting in Godot that would affect the produced project.dll files?
In my limited testing it seems that replacing the generated dll's with whatever i can produce with msbuild on my own works fine - but I don't know if additional stuff is added that would mean it that it now breaks on different platforms/machines.

I guess, if it's the case that supplanting .dll's is safe, then it's a sort of workaround (not completely ofc), even if it's slightly awkward to do.

@paulloz paulloz modified the milestones: 4.3, 4.4 Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing C# defines when exporting with custom feature tags
9 participants