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 smoothstep graphs #9489

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Conversation

Malcolmnixon
Copy link
Contributor

This PR adds two graphs showing the behavior of the smoothstep function with positive and negative ranges.

@AThousandShips AThousandShips added enhancement content:images Issues and PRs involving outdated or incorrect images in articles labels Jun 14, 2024
@BastiaanOlij
Copy link
Contributor

Just for clarity as it's not specified clearly in the OP, Malcolm is adding just images, no documentation changes, here because they are included in the documentation of the smoothstep function inside the main godot repo.

I don't know if there is precedence for this. I don't know if this is the correct way to go about adding images in the class reference, especially seeing the way they are referenced? The images do provide a lot of context to the class reference.

@Malcolmnixon
Copy link
Contributor Author

I decided to merge the graphs into a single comparison image, which also includes showing the zero-range behavior. Additionally the graphs now show the area under the curve to better highlight the function response.

Copy link
Contributor

@BastiaanOlij BastiaanOlij 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, leaving it up to the documentation team to verify this is the way forward.

@mhilbrunner
Copy link
Member

mhilbrunner commented Jul 1, 2024

IIRC this is fine and already done in other parts of the class reference.

Please convert the PNG file to WebP (we're in the process of moving the docs repository over to WebP, see image creation guidelines).

That the linked PR using this image (godotengine/godot#93149) does other stuff than just embedding the image makes it a bit harder to merge, as the main repo PR may get rejected. Please ping me if it gets another approval.

@Malcolmnixon
Copy link
Contributor Author

Please convert the PNG file to WebP (we're in the process of moving the docs repository over to WebP, see image creation guidelines).

The image has been converted to WebP format.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Image looks good to me. Note that the main repository PR needs to be merged first before this PR can be merged.

@Malcolmnixon
Copy link
Contributor Author

Image looks good to me. Note that the main repository PR needs to be merged first before this PR can be merged.

It might be better for this to go in first. If the main repository PR goes in then users trying to access the smoothstep documentation will get missing images. Pulling this in first shouldn't break anything - it'll just be available for use when the main repository merge goes through.

@clayjohn
Copy link
Member

That makes sense. Merging now to be safe

@clayjohn clayjohn merged commit c476471 into godotengine:master Aug 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content:images Issues and PRs involving outdated or incorrect images in articles enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants