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

Added smootherstep() #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

virtualritz
Copy link

Copy link
Owner

@polymonster polymonster left a comment

Choose a reason for hiding this comment

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

Thanks for the addition, I think it will be useful, and also good job adding the vsmootherstep variant as well.

For the sake of this PR I would request that you minimise the changes so it just focuses on the added smootherstep function and changes to the doc comments on smoothstep I had to sift through the minor formatting changes from trimming whitespace, to find all the valid changes.

I should probably sort that out but would prefer to do that in a separate pass, just so that the amount of changes being merged relate solely to smoother step.

Additionally I would suggest to add a test in tests.rs and add the function names to the readme just for completeness.

Once you get that cleaned up I will happily merge this and can push a new version to crates.io.

Thanks for contributing, it's nice to collect new functions they always come in handy :)

@@ -461,14 +463,21 @@ macro_rules! float_impl {
x * x * (3 as Self - 2 as Self * x)
}

fn smootherstep(e0: Self, e1: Self, t: Self) -> Self {
Copy link
Owner

Choose a reason for hiding this comment

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

This could be wrapped into a single statement and then you can do away with the returns:

if t < e0 {  
    Self::zero()
}
else if (t >= e1) { 
    Self::one() 
}
else {
    let x = (t - e0) / (e1 - e0);
    x * x * x * (3 as Self * x * (2 as Self * x - 5 as Self) + 10 as Self)
}

@polymonster
Copy link
Owner

I have trimmed the whitespace in the main branch now, so it should be easier for you to update and reduce the number of changes in the PR.

@virtualritz
Copy link
Author

virtualritz commented May 8, 2023

Most crates require running rustfmt and clippy (and the latter coming out clean) before submitting a PR.

The former will alleviate whitespace issues in the future.

I suggest adding both to a git pre-commit hook.

@polymonster
Copy link
Owner

Most crates require running rustfmt and clippy (and the latter coming out clean) before submitting a PR.

The former will alleviate whitespace issues in the future.

I suggest adding both to a git pre-commit hook.

I am not keen on rustfmt because it messes up some formatting in places so I usually run without.

I regularly run clippy and there are no warnings on this repository.

@virtualritz
Copy link
Author

virtualritz commented May 8, 2023

I am not keen on rustfmt because it messes up some formatting in places so I usually run without.

That will make it difficult for people who want to contributing bigger sections of code. They have to guess expected formatting from your code.

There is a reason code formatting is considered a solved issue in the Rust world. Personally, after 30 years of C/C++ it's one of the smaller but important details that makes Rust so enjoyable for me.
Just saying.

If you disagree with something cargo fmt would do you can add a rustfmt.toml with your specific constraints to the project root. Not as nice for rustaceans reading the code in your repo as is going with defaults. But definitly better than not running a formatter pre-commit at all.

I just ran it for giggles and the changes are mostly cosmetic. Trailing commas mainly and very few actual wraps/line break/block changes. Except for the matrix stuff, which is expected.

@polymonster polymonster self-requested a review May 8, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants