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 more useful Visual Shader nodes #72664

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented Feb 3, 2023

Part of godotengine/godot-proposals#5106

  • RotationByAxis node
    image

Talked about with @QbieShay:

  • World Position from Depth (depth texture)
    image

  • Screen Normal in World Space
    image

@YuriSizov YuriSizov added this to the 4.x milestone Feb 3, 2023
@paddy-exe paddy-exe force-pushed the additional-vs-nodes branch 2 times, most recently from ec9d294 to 5ab6d67 Compare February 5, 2023 17:18
@paddy-exe paddy-exe changed the title Added RotationByAxis node to Visual Shaders Additional Visual Shader Nodes Feb 5, 2023
@Chaosus
Copy link
Member

Chaosus commented Feb 6, 2023

RotationByAxis node

Isn't it the same as VisualShaderNodeParticleMultiplyByAxisAngle?

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Feb 6, 2023

Isn't it the same as VisualShaderNodeParticleMultiplyByAxisAngle?

Not sure to be honest. This node was part of a proposal by @QbieShay for Spatial Shaders (I think).

@QbieShay
Copy link
Contributor

QbieShay commented Feb 6, 2023

Yes this is for spatial shaders!

@paddy-exe
Copy link
Contributor Author

Fixed the depth-ndc for GLES3 just like in #65160

@YuriSizov
Copy link
Contributor

You need to update the generated documentation, we're in the 4.1 stretch now 😎

@paddy-exe paddy-exe requested a review from a team as a code owner April 18, 2023 14:36
@paddy-exe paddy-exe requested review from a team and removed request for a team July 21, 2023 19:59
@QbieShay
Copy link
Contributor

Hey, i tried to use the rotation by axis thigng but it didnt work out

image

image

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jul 24, 2023

I rebased my changes on top and my example still works here:
image

Doesn't seem to change when I use your Node setup (still works):
image

Also @QbieShay:
Are you happy with the changes (see my last commit and comment)?
Additionally, should I add the option to input the angle both in radian as well as degrees or only in radians?

@QbieShay
Copy link
Contributor

I think perhaps it could be a tickbox on the node instead? I think having 2 inputs would be confusing. And perhaps by default it could use CUSTOM.x if no value is supplied?

@paddy-exe
Copy link
Contributor Author

I think perhaps it could be a tickbox on the node instead? I think having 2 inputs would be confusing.

Wouldn't an options drow-down be better than a tick box UX-wise?

And perhaps by default it could use CUSTOM.x if no value is supplied?

I think this could get very confusing for a lot of users as it's not shown in the UI what the default value is. So I would be personally against implementing this. It could be still added later but doing it the other way around would break compatability I assume.

@QbieShay
Copy link
Contributor

I would imagine that rotating particles would be the most basic case but actually probably not, you're right. And you're right about the option drop down too ^^ although maybe it can just be radians. If everything is already in radians we don't need to support degrees. Maybe in the future a float param that auto-converts degrees would be better?

@paddy-exe
Copy link
Contributor Author

And you're right about the option drop down too ^^ although maybe it can just be radians. If everything is already in radians we don't need to support degrees. Maybe in the future a float param that auto-converts degrees would be better?

Yes, IIRC everything is handled as radians. I do find degrees better to use but that can be discussed in a proposal some other time.

So all in all, everything should be finished then, right? Can this be merged?

@QbieShay
Copy link
Contributor

Yep. Thank you for this work.

@YuriSizov YuriSizov changed the title Additional Visual Shader Nodes Add more useful Visual Shader nodes Jul 25, 2023
@YuriSizov
Copy link
Contributor

Could you squash commits and amend the commit message?

@paddy-exe paddy-exe force-pushed the additional-vs-nodes branch 2 times, most recently from 0404270 to 79ff318 Compare July 25, 2023 20:04
@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jul 25, 2023

Could you squash commits and amend the commit message?

Squashed and rebased :)

Edit: I think the broken CI is because of a compatability break not connected to this PR

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 25, 2023

No, it's just a "normal" issue because "No space left on device".

PS. You didn't amend the commit message, btw 🙃

* RotationByAxis Visual Shader Node added
* WorldPositionFromDepth Visual Shader Node added
* ScreenNormalWorldSpace Visual Shader Node added
@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jul 25, 2023

No, it's just a "normal" issue because "No space left on device".

What do you mean by "no space left on device"? Is that a GitHub CI error still?

PS. You didn't amend the commit message, btw 🙃

Ah damn... my bad :/ I usa a visual git editor and thought I used the correct command... Is there something specific I need to add? I reworked it now to tell the changes in bullet points

@YuriSizov
Copy link
Contributor

What do you mean by "no space left on device"? Is that a GitHub CI error still?

Yes, it's an issue with CI itself, not with our project or your PR.

Is there something specific I need to add?

No, it just doesn't follow the commit message style preferred by the project.

@YuriSizov YuriSizov merged commit 1ad95f2 into godotengine:master Jul 26, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks, great work!

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.

6 participants