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#: Make include scripts contents an export option #72896

Merged

Conversation

RedworkDE
Copy link
Member

Includes and requires #72895
Resolves a TODO comment about this, but really this things is just a detour from me looking into embedding the C# outputs into the pck (which really should be such an option).
For backwards compatibility, the sources are preserved if either the project settings or the export option is set. But if this somehow makes it before 4.0 or the minor breakage is acceptable (I don't really think that there is anyone actually depending on having the C# sources available at runtime), the last commit completely removes the project setting. I will either squash or remove it depending on reviews.

@RedworkDE RedworkDE requested review from a team as code owners February 8, 2023 15:20
@RedworkDE RedworkDE force-pushed the net-include-scripts-export-option branch 2 times, most recently from 3b5c09f to 3efa1d6 Compare February 8, 2023 15:34
@neikeq
Copy link
Contributor

neikeq commented Feb 12, 2023

Oh, does this implement a way for export plugins to register custom export settings that appear in the export dialog?

@RedworkDE
Copy link
Member Author

Yes, that is the linked PR / first commit here.

image
(The second option is not part of this PR)

@neikeq
Copy link
Contributor

neikeq commented Feb 13, 2023

That's really great. This is something I always wanted to implement. Currently, there are few C# export settings but in the future there will be more.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 13, 2023
@RedworkDE RedworkDE force-pushed the net-include-scripts-export-option branch 2 times, most recently from 38fe1d9 to d6c6d1d Compare February 13, 2023 16:23
@raulsntos
Copy link
Member

The Core PR (#72895) should be merged first, then this PR rebased. Since they're both in the 4.1 milestone, @neikeq are we OK with the breaking change in commit d6c6d1dab7c7b207927a8bccf602bec8bea1e49e that removes the dotnet/export/include_scripts_content ProjectSetting?

@RedworkDE
Copy link
Member Author

Re: breaking change: On github I can find 10 repos that have enabled this project setting: https://github.com/search?q=export%2Finclude_scripts_content%3Dtrue+path%3Aproject.godot&type=code
Of those, only https://github.com/Srynetix/godot-nature-of-code/blob/master/chapters/SceneExplorer.cs#L188-L197 has actually found a use for it, the others, as far as I can tell, only have the option enabled but don't actually use the contents.

@RedworkDE RedworkDE force-pushed the net-include-scripts-export-option branch from d6c6d1d to e0387c8 Compare April 7, 2023 17:20
@RedworkDE RedworkDE requested review from a team as code owners April 7, 2023 17:20
@RedworkDE RedworkDE force-pushed the net-include-scripts-export-option branch from e0387c8 to 0407f41 Compare April 7, 2023 18:25
@YuriSizov
Copy link
Contributor

#72895 was merged, so this needs its rebase. You should also squash commits, the one removing the setting doesn't look like it needs to be standalone.

Regarding removing the setting, this is okay, generally speaking. But I'll leave it to @godotengine/dotnet to decide :)

@RedworkDE RedworkDE force-pushed the net-include-scripts-export-option branch from 0407f41 to 34b4128 Compare April 18, 2023 12:28
@RedworkDE
Copy link
Member Author

Since nobody was really objecting to removing the (recently renamed) project setting, I removed it, instead of having this be that odd one out setting that can be toggled from two places.

@neikeq
Copy link
Contributor

neikeq commented Apr 18, 2023

I'm fine with removing the setting, since this is a very niche option anyway.

@YuriSizov YuriSizov merged commit 06f5b09 into godotengine:master Apr 18, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@RedworkDE RedworkDE deleted the net-include-scripts-export-option branch April 18, 2023 18:41
@albinaask
Copy link
Contributor

Would this change mean that C# scripts from different pcks can coexist and cooperate? Like if you had DLCs or the like?

@RedworkDE
Copy link
Member Author

Would this change mean that C# scripts from different pcks can coexist and cooperate? Like if you had DLCs or the like?

In its current state no, but it can be the basis for a future system like that.

@albinaask
Copy link
Contributor

I see. Thank you!

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.

None yet

5 participants