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

Add built-in GUI to display license notices (reverted) #79599

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jul 18, 2023

Press Ctrl/Cmd + Shift + L (ui_toggle_licenses_dialog built-in action) to show/hide the notices dialog.

The dialog can be shown via script using SceneTree.licenses_dialog_visible = true|false. Since the default shortcut is not usable on mobile platforms, it is recommended to create a button that sets SceneTree.licenses_dialog_visible to true when pressed in your project's menus.

PS: I believe this is the first built-in high-level GUI component we make available in projects. It'll be interesting to see what can be done in the future with this approach.

Fun fact: This is the first time I add a new node to Godot in C++ 🙂

Testing project: test_licenses_dialog.zip

Preview

Screenshot_20230718_062434

TODO

  • Try using Label instead of RichTextLabel, so that this works in builds using disable_advanced_gui=yes. I've purposefully avoided advanced GUI nodes for this dialog, but had to switch to RichTextLabel because Label was extremely slow to shape the text.
    • I don't even need RichTextLabel BBCode here, and I'm surprised RichTextLabel is so much faster in this use case (easily 100× faster in a debug build). cc @bruvzg

@Calinou Calinou requested review from a team as code owners July 18, 2023 05:02
@Calinou Calinou added this to the 4.x milestone Jul 18, 2023
@Calinou Calinou force-pushed the add-license-notices-gui branch 3 times, most recently from dfff79d to 7ec3b9e Compare July 18, 2023 05:39
@Sauermann
Copy link
Contributor

Instead of basing the new node-type on CanvasLayer, I suggest to base the node on Window or Popup or one of their descendants.
The reason for this is, that when there is a different Window open, then it will overlap the license notices.
Also when a different CanvasLayer with a layer > the layer of your node, then your node will be hidden behind the nodes of the other CanvasLayer.
I'm basing this on the assumption, that the license notices should be visible, no matter what other content is currently displayed.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 18, 2023

Any reason the new class is exposed? It doesn't need to, the relevant documentation can be inside licenses_dialog_visible description.
If we could somehow get rid of the unhandled_input override, we wouldn't need any new class at all, but I think it's not possible.

@Sauermann
Copy link
Contributor

Sauermann commented Jul 18, 2023

How about using AcceptDialog? It already handles input events for closing.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 18, 2023

Ah for some reason I though the method is used for the shortcut. Yeah, it can be replaced with a plain AcceptDialog. If you need to do something extra in set_licenses_dialog_visible(false), you can connect close_requested/confirmed or visibility_changed changed signal.

@MewPurPur
Copy link
Contributor

MewPurPur commented Jul 18, 2023

SVG for the dialog that's 1 kB smaller:

<svg height="16" width="16" xmlns="http://www.w3.org/2000/svg"><path d="M1.3 12.362a1.041 1.041 0 0 0 0 1.465l.865.874a1.041 1.041 0 0 0 1.465 0l2.935-2.944a1.041 1.041 0 0 0 0-1.505l2.07-2.07 1.02 1.02a.38.38 0 0 0 .549 0l-.242.242a.784.784 0 0 0 0 1.1l.457.457a.784.784 0 0 0 1.097 0l3.3-3.3a.784.784 0 0 0 0-1.1l-.457-.457a.784.784 0 0 0-1.1 0l-.242.242a.38.38 0 0 0 0-.549l-2.815-2.815a.38.38 0 0 0-.549 0l.242-.242a.784.784 0 0 0 0-1.1l-.457-.457a.784.784 0 0 0-1.1 0l-3.3 3.3a.784.784 0 0 0 0 1.1l.457.457a.784.784 0 0 0 1.1 0l.242-.242a.38.38 0 0 0 0 .55l1.019 1.02-2.07 2.07a1.041 1.041 0 0 0-1.553 0z" fill="#8eef97"/></svg>

@Calinou
Copy link
Member Author

Calinou commented Jul 18, 2023

Any reason the new class is exposed? It doesn't need to, the relevant documentation can be inside licenses_dialog_visible description.

I thought I was required to expose it, but it looks like it works fine if it's not exposed. I've unexposed the class as a result.

How about using AcceptDialog? It already handles input events for closing.

It requires advanced GUI functionality, which is disabled when building Godot with disable_advanced_gui=yes. My goal is to have this dialog working in every Godot build, even if you disable advanced GUI which is frequently done for custom mobile/web builds to optimize binary size.

These are the nodes that are unavailable when building with disable_advanced_gui=yes:

#ifndef ADVANCED_GUI_DISABLED
GDREGISTER_CLASS(FileDialog);
GDREGISTER_CLASS(PopupMenu);
GDREGISTER_CLASS(Tree);
GDREGISTER_CLASS(TextEdit);
GDREGISTER_CLASS(CodeEdit);
GDREGISTER_CLASS(SyntaxHighlighter);
GDREGISTER_CLASS(CodeHighlighter);
GDREGISTER_ABSTRACT_CLASS(TreeItem);
GDREGISTER_CLASS(OptionButton);
GDREGISTER_CLASS(SpinBox);
GDREGISTER_CLASS(ColorPicker);
GDREGISTER_CLASS(ColorPickerButton);
GDREGISTER_CLASS(RichTextLabel);
GDREGISTER_CLASS(RichTextEffect);
GDREGISTER_CLASS(CharFXTransform);
GDREGISTER_CLASS(AcceptDialog);
GDREGISTER_CLASS(ConfirmationDialog);
GDREGISTER_CLASS(SubViewportContainer);
GDREGISTER_CLASS(SplitContainer);
GDREGISTER_CLASS(HSplitContainer);
GDREGISTER_CLASS(VSplitContainer);
GDREGISTER_CLASS(GraphNode);
GDREGISTER_CLASS(GraphEdit);
OS::get_singleton()->yield(); // may take time to init
bool swap_cancel_ok = false;
if (DisplayServer::get_singleton()) {
swap_cancel_ok = GLOBAL_DEF_NOVAL("gui/common/swap_cancel_ok", bool(DisplayServer::get_singleton()->get_swap_cancel_ok()));
}
AcceptDialog::set_swap_cancel_ok(swap_cancel_ok);
#endif

Window and Popup base classes are available though.

SVG for the dialog that's 1 kB smaller:

Thanks 🙂 I've incorporated it and added you as a co-author.

@Calinou Calinou force-pushed the add-license-notices-gui branch 2 times, most recently from 085fed9 to 3e201fe Compare July 18, 2023 17:24
@KoBeWi
Copy link
Member

KoBeWi commented Jul 18, 2023

Window and Popup base classes are available though.

Then it could be a Window. Although it doesn't really matter if the class is not exposed.

@Calinou Calinou force-pushed the add-license-notices-gui branch 3 times, most recently from b85fa26 to 201064e Compare July 19, 2023 01:56
@KoBeWi
Copy link
Member

KoBeWi commented Jul 19, 2023

I don't even need RichTextLabel BBCode here, and I'm surprised RichTextLabel is so much faster in this use case (easily 100× faster in a debug build).

Pretty sure they are both as fast, but RTL is shaped in thread. If you could divide the whole text into smaller labels, you could add them one by one inside NOTIFICATION_PROCESS.

@Calinou
Copy link
Member Author

Calinou commented Mar 8, 2025

There is a precedent of built-in license dialogs in frameworks, so that legal compliance happens automatically. Try Ctrl + Alt + Middle click in any wxWidgets application (such as FileZilla) and see 🙂

In my experience, it's very common for smaller projects (such as gamejam games) to forget mentioning third-party notices, so anything that can make basic compliance happen out of the box is welcome.

I wish the team could consider an alternative approach for advanced users to opt out of the built-in solution, perhaps a build option.

Using Project > Tools > Engine Compilation Configuration Editor, it should be possible to disable the LicensesDialog node (although I haven't tested it and it might not compile with the class disabled right now).

As it stands, it's not customizable and takes up a shortcut key: developers might not notice that this PR is occupying a shortcut and forget to remove it, which could lead to players accidentally bringing up the license UI during gameplay. This would really disrupt the gaming experience.

I picked a shortcut that's difficult to press by mistake; it's not anywhere near the WASD keys. We could make it also require Alt if we want to be extra sure (so it would be Ctrl + Alt + Shift + L.

I would rather generate a LICENSE file in the export folder.

This file wouldn't be accessible on mobile and web platforms, so it wouldn't be compliant there as there is no way for regular users to access it.

it would be a good idea to document such behavior clearly for the developers to avoid undesired un-styled elements appearing in the Game and breaking UI navigations.

This licenses dialog is supposed to inherit from the default project theme configured in the Project Settings. If it isn't, please open an issue with a minimal reproduction project attached.

Edit: This seems to be the case already: #103820

@TML233
Copy link
Contributor

TML233 commented Mar 8, 2025

There is a precedent of built-in license dialogs in frameworks, so that legal compliance happens automatically. Try Ctrl + Alt + Middle click in any wxWidgets application (such as FileZilla) and see 🙂

In my experience, it's very common for smaller projects (such as gamejam games) to forget mentioning third-party notices, so anything that can make basic compliance happen out of the box is welcome.

A game engine is different from a regular tool app. The end-user of a game engine will be impacted by a built-in license dialog, but the end-user of tools like FileZilla will not. Since game developers typically want to have full control with what they want to present to the player.
If the engine team wants to automatically mention the third-party notices, then an auto-generated LICENSE file when the game is exported would work far better than a dialog in game.
Think of this: a developer won't know there's a license window in their game if you don't tell them. But they will know about license if they are generated in their exported game folder. And even better, any of his players would know about the third-party licenses when they downloads the game.

@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Mar 8, 2025

and it might not compile with the class disabled right now

Then I guess a follow-up PR that implements the disable_builtin_license_gui scons option would be great (disable_builtin_license_gui defaults to no in official builds).

@maidopi-usagi
Copy link
Contributor

IMO It may not be easily pressed by mistake but for some application-like usages(for example, my undergoing modeling tool project), there's still possibility to conflict with user's preconfigured keymaps, as Ctrl+Shift+L seems like a key combination that not so rarely used in content creation cases.

@SheepYhangCN
Copy link
Contributor

This feature should NOT be default enable
It should be a module only add to the game when developer want
We don't want these things that we might never use in regular projects

In short, it is not common enough to add as a built-in feature

By the way, I can trigger two explorer windows with Ctrl + Alt + Shift + L
Even the rarest shortcut keys are not uncommon when there are more users and applications

@bruvzg
Copy link
Member

bruvzg commented Mar 8, 2025

Then I guess a follow-up PR that implements the disable_builtin_license_gui scons option would be great (disable_builtin_license_gui defaults to no in official builds).

If you handle the license display yourself, you can simply unbind the default shortcut in the project settings. What's the purpose of having a build option?

@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Mar 8, 2025

What's the purpose of having a build option?

For reducing unwanted code size, similar to all other optional scons options Godot provides.

@bruvzg
Copy link
Member

bruvzg commented Mar 8, 2025

For reducing unwanted code size, similar to all other optional scons options Godot provides.

The amount of code added is negligible, it does not make any sense to have a dedicated option for it. It's only a dialog code, the licenses text itself was always included.

@YeldhamDev
Copy link
Member

For reducing unwanted code size, similar to all other optional scons options Godot provides.

I doubt that compiling this out would give any meaningful size reductions. My PR to compile out the MovieWriter for example, only reduced 0.05 MBs. And I'm quite sure that feature contains more code than this.

@akien-mga
Copy link
Member

I'd ask that users be more mindful of how you provide feedback on PRs.

5 users jumping out or the blue on a PR to criticize it within 30 min feels quite toxic, coordinated, and out of proportion for what this PR does and how much it would actually impact you. And the PR hasn't even been in a dev snapshot to get actually testing.

It's fine to provide feedback and even to question whether a PR is a good implementation or even a good idea, but please read the room and don't double down when others already expressed your points word for word.

@Delsin-Yu
Copy link
Contributor

The amount of code added is negligible / would give any meaningful size reductions

You do have the point; I give up on that part.

The license shortcut is not a common sense among the players and on web/mobile

Then what should we do for platforms where the shortcuts are not an option? The whole idea of embedding the License window is to make sure there is a way to toggle up the GUI right?

@akien-mga
Copy link
Member

The license shortcut is not a common sense among the players and on web/mobile

Then what should we do for platforms where the shortcuts are not an option? The whole idea of embedding the License window is to make sure there is a way to toggle up the GUI right?

There's an API you can call to show/hide that dialog:

SceneTree::set_licenses_dialog_visible

So the main way I would envision users using this is by adding a button in their game credits to show the dialog.

The rationale for having this dialog built-in and not just leave it to all users to do manually is that it's quite a lot of work to make a dialog like this, and not something that we think users reasonably want to spend days of development on.

Since all Godot users are required to document license details, we figured that having a tool like this can help them do it without too much effort. Of course users are free to choose not to use it, or make their own detailed credits covering all third-party licenses in Godot.

@Delsin-Yu
Copy link
Contributor

it within 30 min feels quite toxic, coordinated, and out of proportion

Random users see the merge message of this PR from the Git branch history and send it across their circle of influence. During the discussion, users with opinions choose to comment on this PR. This process propagates and ends up in what you see as a burst. I do agree people should be mindful about their wording.

@TML233
Copy link
Contributor

TML233 commented Mar 8, 2025

So the main way I would envision users using this is by adding a button in their game credits to show the dialog.

The rationale for having this dialog built-in and not just leave it to all users to do manually is that it's quite a lot of work to make a dialog like this, and not something that we think users reasonably want to spend days of development on.

Of course users are free to choose not to use it, or make their own detailed credits covering all third-party licenses in Godot.

Putting it as an official add-on accomplishes the same thing. This dialog is not eligible to be in core even for godot-proposals. It's too much manual work to set it up properly. Inexperienced developers and regular players won't notice it. Experienced developers don't need it to be in core when they can do it by themselves or download the dialog via the asset marketplace.

@badsectoracula
Copy link
Contributor

As a minor convenience, it might be useful to separate the part of the code that generates the license text to its own method so that people can make their own UI for the license text styled as they want (or placed where they want it, e.g. at a credits screen) but without having to copy/paste or generate the text themselves.

@bruvzg
Copy link
Member

bruvzg commented Mar 8, 2025

As a minor convenience, it might be useful to separate the part of the code that generates the license text to its own method so that people can make their own UI for the license text styled as they want

It already exists:
Engine.get_license_text
Engine.get_license_info
Engine.get_copyright_info

@zaevi
Copy link
Contributor

zaevi commented Mar 8, 2025

I think this PR could be published as an official addon that developers can easily install and enable it.

By the way, my Ctrl+Alt+L, Ctrl+Shift+L and Ctrl+Alt+Shift+L shortcuts are already in use by other applications. The key L is commonly used for locking applications or the system, so it's not uncommon to be occupied.

@scgm0
Copy link
Contributor

scgm0 commented Mar 8, 2025

The license shortcut is not a common sense among the players and on web/mobile

Then what should we do for platforms where the shortcuts are not an option? The whole idea of embedding the License window is to make sure there is a way to toggle up the GUI right?

There's an API you can call to show/hide that dialog:

SceneTree::set_licenses_dialog_visible

So the main way I would envision users using this is by adding a button in their game credits to show the dialog.

The rationale for having this dialog built-in and not just leave it to all users to do manually is that it's quite a lot of work to make a dialog like this, and not something that we think users reasonably want to spend days of development on.

Since all Godot users are required to document license details, we figured that having a tool like this can help them do it without too much effort. Of course users are free to choose not to use it, or make their own detailed credits covering all third-party licenses in Godot.

Since not all platforms support shortcut keys, and we can already control their visibility through code, I think it's reasonable to remove the default shortcut key assignments.

@TML233
Copy link
Contributor

TML233 commented Mar 8, 2025

My point is not about shortcuts or something minor, but the purpose of this PR and how well it works.

  • If the intention of this PR is to let developers be-aware of the license thing, then the team should educate the developers in the docs about the license thing, or let the developers find out that the license thing exists via a auto-generated LICENSE file when they are developing or done developing the game.
  • If the intention is to let the users have a way to view the license, then this license dialog still doesn't do the work. The license shortcut is not a common sense among the players. On Web or Mobile, developers still need to provide a button to open it up.
    • If developers don't even know this dialog is a thing, then they won't provide a button to do that.
    • If a developer knows about the license thing and is willing to show it in the game, they will make their own dialogs that fits into their game and is available on any platform.

Provide this dialog as an add-on would be a much more appropriate way. Developers would know that was a thing and integrate it into their project. All the team need to do is to educate developers about the importance of it in the docs.

I don't think this PR accomplished any of its possible intentions well enough to stay in core.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 8, 2025

The license shortcut is not a common sense among the players.

It's not about whether they know it or not. If the dialog exists and it's accessible, it makes you automatically comply with the license.

@TML233
Copy link
Contributor

TML233 commented Mar 8, 2025

The license shortcut is not a common sense among the players.

It's not about whether they know it or not. If the dialog exists and it's accessible, it makes you automatically comply with the license.

The dialog won't be accessible on mobile and web without a keyboard if developers don't implement a button to open it up. On PC, an auto-generated LICENSE file is already enough to make the developers automatically comply with the license.

@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Mar 8, 2025

and it's accessible, it makes you automatically comply with the license.

I guess the key disagreement is here: The developer for the web and mobile platform needs to manually implement a button to make the window accessible. However, they need to acknowledge that the window exists in the first place. Once they know, they will likely create a license window themselves, making the original intention of the PR awkward.

@bruvzg
Copy link
Member

bruvzg commented Mar 8, 2025

purpose of this PR

Providing zero (or minimal) effort way for license compliance.

99% of users do not care about third-party licenses and will never view it, so developers can avoid spending any time on customizing / making own UI for it.

The dialog won't be accessible on mobile and web without a keyboard if developers don't implement a button to open it up.

Yes, but adding a single button is much less work that making dialog yourself.

then the team should educate the developers in the docs about the license thing

It's documented here https://docs.godotengine.org/en/stable/about/complying_with_licenses.html, not sure what else can be done.

@Delsin-Yu
Copy link
Contributor

not sure what else can be done.

Perhaps adding notifications to the export window?

@adamscott
Copy link
Member

adamscott commented Mar 8, 2025

The PR was merged. Arguing here just leads to bikeshedding.

If you disagree with the current state of the engine, either create a proposal if you want to suggest a new way to deal with this or create an issue.

@godotengine godotengine locked and limited conversation to collaborators Mar 8, 2025
@akien-mga
Copy link
Member

Just noting: We locked the discussion because it was quite bikeshedding and a hot topic (30+ messages in a couple hours). But we hear the feedback and will see how to shape this further to take it into account. I agree there are some aspects we need to reconsider in the current implementation.

@adamscott
Copy link
Member

A related issue was created. #103830

@akien-mga akien-mga changed the title Add built-in GUI to display license notices Add built-in GUI to display license notices (reverted) Mar 10, 2025
@akien-mga
Copy link
Member

We're reverting this for now (#103906) to go back to the drawing board and see what a more consensual implementation could be.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.