-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Redesign the graph editor for visual shaders #85017
Conversation
This looks great! I wonder if the margin headings could be reduced though, as they look massive at the default zoom level currently (they take half the space in single-property nodes). I understand this was done to make heading colors visible at low zoom levels, but maybe a different approach can be used to highlight these at low zoom levels instead. |
Maybe at a certain point while zooming out, the header colour could simply be extended to the entire node? With a sufficiently low zoom factor, the text stops being readable anyway. At that point, the nodes might as well be represented by coloured rectangles. An instant switch without transition might be a little sudden though. |
I see that this PR changes the colors for various types. The type colors should be kept in sync with the icon colors, so I would suggest either changing both or neither. |
89fa254
to
7ebded5
Compare
7ebded5
to
1440938
Compare
Rebased. The connection rim was split from this PR and is now part of #86158 (which this PR depends on). |
1440938
to
e545e68
Compare
Rebased and added a new category and color for particle nodes: @aaronfranke |
@Calinou The header heights are that big for another reason: More space for selecting/grabbing the nodes. Especially at high zoom levels (but not exclusively) this improves the flow since you don't need such a high click accuracy - reducing missclicks. Changing the height dynamically depending on the zoom levels might prove quite difficult and could cause all sorts of problems, so I'd rather not do this. (unfortunately I don't have any other ideas to highlight them at low zoom levels) |
I guess it makes sense to keep the heading heights this way then. |
e545e68
to
ac90cab
Compare
@Geometror In that case I would suggest using the old colors, so that the colors stay in sync until such an icon redesign overhaul can be completed. Also, to comment on one of the colors in your redesign specifically: I'm not a fan of using white or light-gray lines for data. In visual scripting languages, the color white is used for sequencing, including in Godot 3's VisualScript, Unreal Blueprints, The Mirror's visual scripting, and more. I would prefer to avoid inconsistency between visual scripts and visual shaders by ensuring that all data types are using a non-white color. |
@aaronfranke Well, changing the connection colors is an integral part of this redesign, I tried the new header colors together with the old connection colors and it looked horrible. For example, for scalar values it's gray which isn't a saturated color on purpose since scalar connections are the most common type and therefore prevalent in most shader graphs. Having those colored (especially in the cyan used before) makes the graph way too colorful and hurts readability (since it interferes with the header colors). You're right, the color white is often used in scripting languages for control flow/sequencing, but on the other side, it is also used for data (or specifically scalar types) in Blender, Material Maker and the the Unreal material editor (Unity uses colors, but very desaturated). As this redesign is primarily inspired by Blender (to reduce fricition/cognitive overhead) I went with gray. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
fb10e67), it works as expected. It looks great 🙂
One issue I noticed is that if you use the Black (OLED) editor theme preset, unselected nodes will blend with the background:
To combat this, there is a Draw Extra Borders editor setting that is automatically enabled when choosing this theme preset. When this setting is enabled, I suggest adding a gray 1-pixel border around unselected GraphNodes like we do on LineEdit, for instance.
@Calinou While adjusting the theme for the OLED preset, I realized that the contrast of the node borders is generally pretty low. So I played a bit with it: What do you think? (I took the following two screenshots before adjusting the port offset) |
f62003b
to
9ba9274
Compare
9ba9274
to
02a33b6
Compare
Thanks! |
Well done! |
I don't know if this is in the new version but does this new version of the node system support comment grouping for chunks of nodes? It's just an organization thing. For example, you highlight a group of nodes for one part of the script and press a shortcut key (C), that puts that section in a comment bubble. Its easier to see what part of the shader is doing what. |
Do you mean something like this #88014 ? |
Depends on #83785.(merged)Depends on #86158.(merged)Although it does not depend on #83510, it would make sense to merge that PR first.(merged)This PR aims to improve the VisualShader workflow especially when working with complex graphs where it's important that you're able
to quickly parse the graph and get an overview about what each part does (at least roughly). As color is a very powerful instrument to guide your eyes and aid with comprehension of complex structures, we need to carefully apply it.
Comparisons
Comparison using a complex node graph (thanks to @TheRensei for the shader which was very useful for testing/iterating):
Current:
This PR:
Detailed changes
Default
andLegacy
(old connections colors + no distinct colors for categories) theme presetsCustom
color theme allows setting each color manuallySince this is huge change we need to make sure that everything fits, so feel free to give it a try and let me know what you think and what could be improved. There are still some things to be discussed such as the color of Vector3/4 connections (maybe having two colors for Vector3, one for color related nodes and the other one for general vector nodes).
Thanks to everyone who gave such nice and valuable feedback during GodotCon.
Production edit: closes godotengine/godot-roadmap#12