Skip to content

Conversation

@Mickeon
Copy link
Member

@Mickeon Mickeon commented Jun 5, 2025

While making #107143 I discovered that for some ungodly reason EditorExportPreset's get_script_export_mode has been exposed to return an integer, not a value from ScriptExportMode, which was exposed for this exact, singular purpose. This PR fixes that.

This probably breaks compatibility and I am not sure who can review this. Does it count as a fix or an enhancement?

@Mickeon Mickeon added this to the 4.5 milestone Jun 5, 2025
@Mickeon Mickeon requested a review from a team as a code owner June 5, 2025 16:58
@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from 9fe074c to a288fc8 Compare June 5, 2025 17:13
@Mickeon Mickeon requested a review from a team as a code owner June 5, 2025 17:13
}

int script_export_mode = current->get_script_export_mode();
int script_export_mode = int(current->get_script_export_mode());
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is necessary but...?

@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from a288fc8 to edab528 Compare June 5, 2025 19:39
@Mickeon Mickeon requested review from a team as code owners June 5, 2025 19:39
@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch 4 times, most recently from 33d250d to 51da650 Compare June 5, 2025 21:50
@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch 2 times, most recently from fac28f3 to f7fe7ef Compare June 6, 2025 08:06
Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Does changing from int to enum actually break binary compatibility? Seems to me that the FFI representation should be the same 🤔

Or is this a shortcoming of the compatibility mechanism, which treats enum and int as different?

(Obviously it does break source compatibility in situations where GDExtension bindings generate strongly typed enums, but the compatibility is mostly to allow 4.2 extensions to run under 4.4, which should be possible without registering a compatibility method.)

@raulsntos
Copy link
Member

Does changing from int to enum actually break binary compatibility?

It does for the C# bindings. And the compatibility method would not prevent it, because C# does not allow overloads on the return type.

@Repiteo Repiteo modified the milestones: 4.5, 4.6 Sep 8, 2025
@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from f7fe7ef to 36c701b Compare November 6, 2025 19:51
@Mickeon Mickeon requested a review from a team as a code owner November 6, 2025 19:51
@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from 36c701b to 72c04aa Compare November 6, 2025 20:17
@Mickeon
Copy link
Member Author

Mickeon commented Nov 6, 2025

Rebased.
We still need to determine whether we should break compatibility for this (and potentially how to mitigate it in other ways?)

@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from 72c04aa to 4ad3581 Compare November 7, 2025 09:38
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

I think compatibility breakage in this scenario is warranted, but hearing some second opinions would be nice

@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch 2 times, most recently from 01f5e90 to 58d36fe Compare November 19, 2025 08:52
@KoBeWi
Copy link
Member

KoBeWi commented Nov 24, 2025

Reminds me of #108559

The breakage is only relevant when someone had used the method, no? It seems very niche, it's likely fine to change it.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

As long as the compatibility breakage is deemed acceptable, I approve of this.

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

This is a fairly small change, which amusingly requires quite a bit of boilerplate (but is still valid and worth noting). I don't think many users were concerned about the method returning int instead of ScriptExportMode, much less be affected by this change.

In terms of compatibility, in GDScript int is compatible with any enum, so narrowing the return value of a non-virtual method should be safe.

@Repiteo
Copy link
Contributor

Repiteo commented Nov 25, 2025

That's a healthy enough sample size to move forward with a merge (after rebase)

@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from 58d36fe to f662e28 Compare November 25, 2025 19:42
@Mickeon
Copy link
Member Author

Mickeon commented Nov 25, 2025

Rebased.

@Mickeon Mickeon force-pushed the actually-use-script-export-mode-what-the-hell branch from f662e28 to 3b1a62f Compare November 27, 2025 00:39
@akien-mga
Copy link
Member

Needs another rebase. The fast-moving compat file is annoying.

@bruvzg
Copy link
Member

bruvzg commented Dec 2, 2025

The fast-moving compat file is annoying.

Maybe we should reorganize it, it's a constant waste of time since it's always required in applying both changes, and there's no real conflicts to resolve.

If it is one file per-PR it should not cause conflicts: like misc/extension_api_validation/4.5-stable/{PR_NUMBER}.expected, instead of a section in the same file.

@akien-mga akien-mga force-pushed the actually-use-script-export-mode-what-the-hell branch from 3b1a62f to 14ede94 Compare December 3, 2025 08:22
@akien-mga akien-mga merged commit b603be2 into godotengine:master Dec 3, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the actually-use-script-export-mode-what-the-hell branch December 5, 2025 16:35
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.

10 participants