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

Visual Shaders: Make output-ports for vector types expandable by default #82088

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

DennisManaa
Copy link
Contributor

@DennisManaa DennisManaa commented Sep 22, 2023

This PR is a continuation of the work done in: #73041

Changes made

On the left side you can see the state before the changes I made (which is the same as in #73041) and on the right side you can see the state after the changes I made. The following changes were made:

  • Marked in red: Removed the expand buttons from the vector-components.
  • Marked in blue: Renamed the component-ports, to better reflect which vector-component is meant (as suggested by @Norrox).
  • Marked in yellow: Reversed the directions of the collapse/expand-arrows when open/closed, which aligns them with other occurrences of these buttons.
  • Marked in magenta: Fixed outputs of Vector2Parameter, Vector3Parameter, Vector4Parameter and CurveXYZTexture-Nodes.
  • Marked in cyan: Fixed many nodes that weren't working correctly (the preview and output of vector-components were interpreted as if they were vector-outputs, what caused ports of several nodes to behave strangely)

pr_marked_changes_01
pr_marked_changes_02
pr_marked_changes_03
pr_marked_changes_04
pr_marked_changes_05

@DennisManaa DennisManaa requested a review from a team as a code owner September 22, 2023 03:47
@DennisManaa DennisManaa changed the title Visual Shaders: Make ports for vector types expandable by default Visual Shaders: Make output-ports for vector types expandable by default Sep 22, 2023
@Chaosus Chaosus added this to the 4.x milestone Sep 22, 2023
@Chaosus
Copy link
Member

Chaosus commented Sep 22, 2023

Thanks, this PR looks very good. Please squash commits as required by our pipeline (see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#making-changes)

Copy link
Contributor

@QbieShay QbieShay left a comment

Choose a reason for hiding this comment

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

PR looks good to me, my only comment on this is that I find the red(x) syntax very hard to read. I'd rather we remove this change and discuss a bit wheter we want to have some nodes be rgba and some nodes directly xyzw, or find some less visually cluttered ways to represent this.

@Norrox
Copy link
Contributor

Norrox commented Sep 22, 2023

PR looks good to me, my only comment on this is that I find the red(x) syntax very hard to read. I'd rather we remove this change and discuss a bit wheter we want to have some nodes be rgba and some nodes directly xyzw, or find some less visually cluttered ways to represent this.

I like this PR 😁, would it be better to remove red green blue text completely and for example just write X in color red and the port be red as it is?

@AThousandShips
Copy link
Member

AThousandShips commented Sep 22, 2023

example just write X in color red and the port be red as it is

That's not appropriate for accessibility, neither from a visibility perspective or from the perspective of communicating information without relying on color

@QbieShay
Copy link
Contributor

I'd rather get this pr merged soon and discuss on those UI changes separately, possibly in a proposal

@DennisManaa
Copy link
Contributor Author

DennisManaa commented Sep 22, 2023

Thank you very much for your rapid feedback. I'm glad to hear that you like this PR :)
I squashed the commits and reverted the changes regarding the naming of the ports.

For the future, it might be a good idea to consider changing the naming of the vector components in a way that will allow for different names for different nodes. One way to implement this would be to create a new virtual function in the VisualShaderNode class and override this function where needed. This approach would enable us to use appropriate names for every node, such as 'u' and 'v' for nodes that output UVs.

@QbieShay QbieShay modified the milestones: 4.x, 4.2 Sep 22, 2023
…utput port and it's of any vector type

Co-authored-by: QbieShay <[email protected]>
@DennisManaa
Copy link
Contributor Author

DennisManaa commented Sep 22, 2023

Sorry for this change...

I just realized I made a mistake in the commit message. The 'Co-authored-by:' tag wasn't recognized correctly, so I've corrected it. I built on top of QbieShay's work, so it should correctly be represented in the commit! :)

@akien-mga akien-mga merged commit f795e45 into godotengine:master Sep 22, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@DennisManaa DennisManaa deleted the qbie/expand-ports branch September 22, 2023 22:09
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