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#: Document new version defines and remove mentions of old defines #7525

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

RedworkDE
Copy link
Member

The version defines were added in godotengine/godot#78249 and godotengine/godot#78270

The feature defines haven't existed in a long time, and even if godotengine/godot#53920 finally gets anywhere, they will be in a different format, The architecture defines no longer exist either and there doesn't seem to be any plan / demand to reintroduce them.

For the platforms that have multiple defines we should probably just pick one to mention, but I am not sure which of them is preferred.

@raulsntos
Copy link
Member

For the platforms that have multiple defines we should probably just pick one to mention, but I am not sure which of them is preferred.

I wonder why some platforms have multiple defines, probably a leftover from an older version.

For example, the Web platform used to be called JavaScript1, and since Godot adds OS features for each platform it's likely that C# was just emulating this by adding the GODOT_JAVASCRIPT define, then when it was renamed we may have added GODOT_WEB but didn't remove the old one.

Also, at some point there was a html5 OS feature, but that doesn't seem to exist anymore either.

I guess the situation with (GODOT_OSX and GODOT_MACOS) and (GODOT_IPHONE and GODOT_IOS) is similar. These platforms were also renamed (OSX to MacOS and iPhone to iOS)2.

We could probably remove the duplicate defines, although I guess it breaks compatibility. But I agree that we should only mention the could mention only one in the documentation so I'd pick the ones that match the platform name.

Footnotes

  1. https://github.com/godotengine/godot/commit/d20b32186fc192f5e527a1211291b0cb293f4e66

  2. https://github.com/godotengine/godot/commit/8823eae328547991def3b13ee2919291d29a278b

@raulsntos raulsntos added topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement labels Jun 15, 2023
@RedworkDE RedworkDE force-pushed the net-version-defines branch from 0687475 to 17cd80d Compare June 15, 2023 22:09
@RedworkDE
Copy link
Member Author

Ok, I removed the old apple platform defines and used GODOT_WEB as the define for the web platform, and let it do double duty as both the platform type and platform define.

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

@mhilbrunner mhilbrunner merged commit fcc07e1 into godotengine:master Jul 18, 2023
@mhilbrunner
Copy link
Member

Thank you! Merged. And thanks Raul for reviewing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants