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

C#: Add version defines to help users deal with breaking changes #78249

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

RedworkDE
Copy link
Member

Adds an easy way to write C# scripts that work with multiple godot versions with breaking changes between, for example for scripts distributed via the asset library.

Such version defines are pretty common for C# and both the .net runtime and Unity have them in pretty the same form (https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives#conditional-compilation, https://docs.unity3d.com/Manual/PlatformDependentCompilation.html)

While this can largely be done with runtime version checks and untyped code instead, in C# (unlike in GDScript) this is pretty painful and completely different from how code is normally written.

Example Usage:

public string[] GetObjectMetaList(GodotObject obj)
{
#if GODOT4_1_OR_GREATER
  return obj.GetMetaList().Select(e => (string)e).ToArray();
#else
  return obj.GetMetaList();
#endif
}

While it is pretty late for 4.1, this change is pretty much zero risk and the more versions don't support this the less useful this becomes.

The defines have to be manually updated when changing versions. It is probably possible to automatically generate these, but that is unnecessarily complex for a first version.


The defines added are one for the major, minor, and patch of the current version, and then OR_GREATER defines from 0 to the current value for each component, if these defines have existed in that version.

Examples: for 4.3.2: GODOT4, GODOT4_OR_GREATER, GODOT4_3, GODOT4_1_OR_GREATER, GODOT4_2_OR_GREATER, GODOT4_3_OR_GREATER, GODOT4_3_2, GODOT4_3_0_OR_GREATER, GODOT4_3_1_OR_GREATER, and GODOT4_3_2_OR_GREATER
for 5.0.1: GODOT5, GODOT4_OR_GREATER, GODOT5_OR_GREATER, GODOT5_0, GODOT5_0_OR_GREATER, GODOT5_0_2, GODOT5_0_0_OR_GREATER, and GODOT5_0_1_OR_GREATER

@RedworkDE RedworkDE requested a review from a team as a code owner June 14, 2023 23:36
@Calinou Calinou added this to the 4.x milestone Jun 15, 2023
@Calinou
Copy link
Member

Calinou commented Jun 15, 2023

This is great to have, so that C# add-ons can support multiple Godot versions more easily 🙂
Given the compatibility reasons you mentioned, I think this would make sense to cherry-pick to 4.0 too.

Should it be GODOT4_1 or GODOT_4_1? Personally, the latter looks more natural to me.

@raulsntos
Copy link
Member

Should it be GODOT4_1 or GODOT_4_1? Personally, the latter looks more natural to me.

.NET uses NET6_0 so I think we should follow the same pattern.

@RedworkDE
Copy link
Member Author

RedworkDE commented Jun 15, 2023

One the other hand unity uses UNITY_X_Y_Z(_OR_NEWER) so there is precedent for having that under score as well, and I had it like that in my first draft, but it looked kinda long to me and .NET doesn't have it, so I removed it. (the old .NET framework defines don't even have the underscore between components)

It is pretty much a whatever, both work, tho and I don't really have a strong opinion either way.

@akien-mga
Copy link
Member

akien-mga commented Jun 15, 2023

Does C# support giving values to defines and doing simple comparisons in #if checks?

Because we already have a hex-encoded version number that can be used for any complex comparison logic without having to make multiple defines for major, minor, patch and or greater variants of all past releases:

https://github.com/godotengine/godot/blob/33957aee69683cf1f542a8622e5a9efd23070f1c/core/version.h#L52L55

So the example would be:

#if GODOT_VERSION >= 0x040100

For the record that's what third-party C++ modules do.

If that's hard to parse for some, we can also add a macro like #if GODOT_VERSION >= MKVERSION(4, 1, 0) which would compute that number.

Maintaining a list like this manually seems like a hassle, and one more thing to bump everytime we make a release.

Examples: for 4.3.2: GODOT4, GODOT4_OR_GREATER, GODOT4_3, GODOT4_1_OR_GREATER, GODOT4_2_OR_GREATER, GODOT4_3_OR_GREATER, GODOT4_3_2, GODOT4_3_0_OR_GREATER, GODOT4_3_1_OR_GREATER, and GODOT4_3_2_OR_GREATER

About that list, what happens if someone used GODOT4_2_2_OR_GREATER when they upgrade to 4.3.2 which doesn't define it? Or do we need to define all past maintenance releases too? Then what if we make a 4.2.x maintenance release after 4.3.2 is released, and thus it doesn't know about it?

@RedworkDE
Copy link
Member Author

RedworkDE commented Jun 15, 2023

The C# preprocessor is extremely limited, it only supports #if and some options for controlling warnings and source information, but notably no macros, and defines can only exist or not, they cannot have any value.

If they used GODOT4_2_2_OR_GREATER because they use a feature that was added in 4.3 and then backported to 4.2.2 they should have instead checked for #if GODOT4_3_OR_GREATER || GODOT4_2_2_OR_GREATER, otherwise yes that code would no longer be included, but 4.3.2 also isn't really a version that "includes" 4.2.2.

Also if there was a feature that was added in 4.3 and then cherry-picked to 4.2.1 and 4.1.7 the checks would become really complicated, if the patch version defines for previous minor versions were included.
And also only defining patch versions for the current minor keeps the maintenance burden in check (its really just incrementing a few of the numbers and adding one OR_GREATER) and also makes it reasonable to automatically generate these defines. (I just didn't to avoid making this PR any later then it already is)

@akien-mga
Copy link
Member

I see. It's a shame that the C# preprocessor is so limited.

I'm still not sure how people would effectively use patch version defines without break forward compatibility with future minor versions.

Would GODOT4_3_OR_GREATER || GODOT4_2_2_OR_GREATER return true on both 4.3 which doesn't have the 4.2.2 define, and on 4.2.2 which doesn't have the 4.3 define?

@RedworkDE
Copy link
Member Author

Would GODOT4_3_OR_GREATER || GODOT4_2_2_OR_GREATER return true on both 4.3 which doesn't have the 4.2.2 define, and on 4.2.2 which doesn't have the 4.3 define?

Exactly.

Basically for C# #if every define is wrapped in a defined(X) and also the only operators that are supported are ||, &&, !, and parentheses. It is powerful enough to do this like this, where the interface of some dependency changes or conditional optimizations, but you can't really do anything more than that.

@akien-mga akien-mga modified the milestones: 4.x, 4.1 Jun 15, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me given the constraints of the C# preprocessor.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

LGTM. We should consider backporting to 4.0 as Calinou suggested, in which case we'll want to add those defines here as well.

@akien-mga akien-mga merged commit 7f5ef95 into godotengine:master Jun 15, 2023
@RedworkDE RedworkDE deleted the net-version-define branch June 15, 2023 13:28
@akien-mga
Copy link
Member

Thanks!

@magian1127
Copy link
Contributor

magian1127 commented Jun 17, 2023

vs 2022 17.6.2 + dotnet 7
Invalid compiler argument, "/define:

After I tried to remove the newline, it was all right

<GodotDefineConstants>GODOT;GODOT4;GODOT4_OR_GREATER;GODOT4_1;GODOT4_1_OR_GREATER;GODOT4_0_OR_GREATER;GODOT4_1_0;GODOT4_1_0_OR_GREATER;</GodotDefineConstants>

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.

5 participants