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 shader ports are expandable by default for vector types #73041

Closed
wants to merge 1 commit into from

Conversation

QbieShay
Copy link
Contributor

@paddy-exe
Copy link
Contributor

Works as expected:
image

@Norrox
Copy link
Contributor

Norrox commented Feb 10, 2023

Are we sure we want red green blue? we take in x y z? they are the same sure, but why not name them r(x) g(y) b(z) or red(x) green(y), something like that? :)

@QbieShay
Copy link
Contributor Author

QbieShay commented Feb 10, 2023

I think that could be done in a future PR, this just uses the default implementation

fire
fire previously approved these changes Feb 10, 2023
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

People tested the changes and the code is decent

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.

Looks good, however these expand buttons (which are toggleable, but do nothing) shouldn't be there, should they?
grafik
The ColorConstant node on the right does not have them.

Edit: Also I just noticed in @paddy-exe's screenshot that there seems to be some weird thing happening with the component ports of Vector3Parameter (also reproducible with Vector2/4Parameter):
grafik

@QbieShay
Copy link
Contributor Author

that's really strange, it shouldnt show for them

Chaosus
Chaosus previously approved these changes Mar 1, 2023
@Chaosus
Copy link
Member

Chaosus commented Mar 1, 2023

Ah, as far as I remember I did not make this change in original PR, cuz I did not manage it to work correctly with nodes with multiple vector ports.

The code for _add_node in VisualShaderEditor need to be fixed to make it work properly.

@YuriSizov
Copy link
Contributor

So despite the two approvals, this still needs some work, correct?

@QbieShay
Copy link
Contributor Author

Yeah i haven't gotten back to this yet. Anybody that wants to pick up this work is welcome, i won't have time for a bit.

@akien-mga akien-mga marked this pull request as draft April 25, 2023 14:01
@QbieShay QbieShay modified the milestones: 4.1, 4.2 May 29, 2023
@YuriSizov YuriSizov dismissed stale reviews from Chaosus and fire July 7, 2023 12:08

Stale review

@DennisManaa
Copy link
Contributor

Hey there, I'd like to pick up this PR, if still possible :)

@QbieShay
Copy link
Contributor Author

Sure go ahead! I haven't had the energy to continue on this so that'd be very welcome 👍

@DennisManaa
Copy link
Contributor

Hey again, just would like to mention the new PR, where I finished the work on this issue :)

#82088

@QbieShay
Copy link
Contributor Author

Superseded by #82088

@QbieShay QbieShay closed this Sep 22, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants