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

Make GraphEdit toolbar more customizable #81582

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Sep 12, 2023

Depends on #81551 (drafting for now).
This should address the issues mentioned in godotengine/godot-proposals#7061, possibly enough to close the proposal.

I'm introducing a set of exposed properties to hide parts of the menu bar, and the menu bar itself. Previously existing properties to hide the arrange button and to show the zoom label are grouped together with the new properties in the inspector. The property to hide the arrange button has been renamed, because its appearance in the inspector can be very misleading:

godot windows editor dev x86_64_2023-09-12_19-36-03

Now the property is called hide_arrange_button, and its methods are set_hide_arrange_button and is_hiding_arrange_button. Similar naming scheme was used for the new properties. The property to show the zoom label remains unchanged.

This can be discussed further because the problem of negative and positive names is a pretty annoying one. But for now this is my proposal.

godot.windows.editor.dev.x86_64_2023-09-12_20-03-27.mp4

Additionally I addressed a usability issue with the toolbar that I had noticed long ago. Since the toolbar didn't have a background and could easily appear on top of various text and icons, it could be hard to read and use under certain circumstances:

godot windows editor dev x86_64_2023-09-12_19-07-41

So I added a semi-transparent background (both in the default theme and in the editor):

godot windows editor dev x86_64_2023-09-12_19-28-03 godot windows editor dev x86_64_2023-09-12_19-30-52

PS. There are some changes in unrelated files because I've cleaned up includes in graph_edit.h.

@lostdisplay
Copy link

Great stuff! Is it possible to give the user the option to hide the scrollbars too?

@YuriSizov
Copy link
Contributor Author

@lostdisplay Sure, but it would be a considerable usability downgrade to do so if panning and zooming is still allowed. What would be the use case?

@lostdisplay
Copy link

@YuriSizov I can think of a use case, and that's for my personal project 😅, an app for taking notes with an infinite whiteboard. The scroll bars would just be a hindrance. Right now I'm connecting the visibility_changed signal of the internal scrollbars to a function that hides them again, which is hacky at best.

But to be honest, this has a very limited use case. I've seen some people using GraphEdits for skill trees though?

@YuriSizov
Copy link
Contributor Author

I've seen some people using GraphEdits for skill trees though?

I haven't, and I'm not sure GraphEdit is that suitable for more simplistic game UIs. But I guess I've seen games where such UIs are done without any scroll indicators. I think it's a material for a different PR, but yeah, it should be doable, since it's GraphEdit that hides and shows them.

@YuriSizov YuriSizov force-pushed the graph-toolbarniceness branch from e6577ca to 4edb931 Compare September 14, 2023 14:25
@YuriSizov YuriSizov marked this pull request as ready for review September 14, 2023 14:25
@YuriSizov YuriSizov requested review from a team as code owners September 14, 2023 14:25
@YuriSizov
Copy link
Contributor Author

Rebased on master and also added compatibility methods. Properties cannot be added for compatibility at the moment. Something to look into in the future. GDExtensions don't have them, but C# might need us to do this gracefully. Can also be useful if we want to expose compatibility code to GDScript.

@raulsntos
Copy link
Member

C# might need us to do this gracefully.

That's right, we would have to add the old properties to the Compat.cs file if we want to avoid breaking compat.

That said, since GraphEdit is marked experimental, and we are already breaking compat in the GraphNode API, is there any point to providing these compat methods/properties?

@YuriSizov
Copy link
Contributor Author

That said, since GraphEdit is marked experimental, and we are already breaking compat in the GraphNode API, is there any point to providing these compat methods/properties?

Outside of the automated system that we have, no, I don't think so. But for GDExt we want to keep binary compatibility as intact as possible, so methods at least are worth adding when we remove or rename them.

@raulsntos
Copy link
Member

I don't disagree with that. My point was that if we are already breaking binary compatibility in the GraphNode API, adding these compat methods won't help those users, right? They would still be affected by the break.

@YuriSizov YuriSizov force-pushed the graph-toolbarniceness branch from 4edb931 to 8a0ea2f Compare September 14, 2023 15:04
@YuriSizov
Copy link
Contributor Author

My point was that if we are already breaking binary compatibility in the GraphNode API, adding these compat methods won't help those users, right? They would still be affected by the break.

Most of it was renames, so a compatibility method that routes to the new method should be sufficient for GDExtension users who may be using them. That said, I'm not particularly sure where the line would be in this specific case.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Overall solid work, the visual usability improvement looks nice!
I was actually just planning to implement this after I took another look at my GraphEdit refactoring todo list. Luckily I noticed your PR before that :)

scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
scene/gui/graph_edit.cpp Show resolved Hide resolved
@YuriSizov
Copy link
Contributor Author

YuriSizov commented Sep 26, 2023

Changed all toolbar properties to be show_* (with show_zoom_label being the only one off by default, which is its current state).

godot windows editor dev x86_64_2023-09-26_13-21-53

With the flat button changes the toolbar currently looks like this

godot windows editor dev x86_64_2023-09-26_13-21-36

image
image

Light mode definitely needs more work overall, but it should still benefit from these changes

@YuriSizov YuriSizov force-pushed the graph-toolbarniceness branch from ff8a862 to 1e8b8a8 Compare September 26, 2023 17:28
@elmanjar13
Copy link

Thats' good!!!!

Also adds a semi-transparent background
to make the toolbar stand out in front of nodes.
@YuriSizov YuriSizov force-pushed the graph-toolbarniceness branch from 1e8b8a8 to b07c664 Compare October 2, 2023 11:43
@akien-mga akien-mga merged commit 57a6813 into godotengine:master Oct 2, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov deleted the graph-toolbarniceness branch October 2, 2023 13:36
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.

6 participants