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

WIP Histogram #54

Merged
merged 11 commits into from
May 31, 2024
Merged

WIP Histogram #54

merged 11 commits into from
May 31, 2024

Conversation

magnetophon
Copy link
Contributor

As requested in the chat, a PR for this WIP code.

Copy link
Owner

@exa04 exa04 left a comment

Choose a reason for hiding this comment

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

Looking good so far! Here are some suggestions :)

src/visualizers/histogram.rs Outdated Show resolved Hide resolved
src/visualizers/histogram.rs Outdated Show resolved Hide resolved
src/visualizers/histogram.rs Outdated Show resolved Hide resolved
src/utils/buffers/histogram_buffer.rs Outdated Show resolved Hide resolved
src/visualizers/histogram.rs Outdated Show resolved Hide resolved
@magnetophon
Copy link
Contributor Author

magnetophon commented May 31, 2024

Thanks for the feedback and your great help!
I've implemented all your suggestions, from both here and our chats.
Let me know if I missed anything.

Next steps, as far as I can tell, are improving the decay scaling and making it adjustable.
Should I implement FillModifiers?
If yes, I'd need some guidance. :)

Anything else?

// let largest = self.buffer.iter().fold(std::f32::MIN, |a,b| a.max(*b));
let mut largest = 0.0;

for i in 0..nr_bins {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I turn this into an iter?
Would that improve performance?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think you really need to do that. It would also mean creating a new iterator struct for the histogram buffer and implementing iter() or into_iter() for the buffer. It wouldn't really have any effect on performance, I think.

@exa04
Copy link
Owner

exa04 commented May 31, 2024

I just tried your histogram example and it seems to work really well. Awesome!

Should I implement FillModifiers?

You can just leave that up to me :)

@magnetophon
Copy link
Contributor Author

Cool, thanks!

@magnetophon
Copy link
Contributor Author

OK, done.

Shall I make it all into a single commit?

@exa04
Copy link
Owner

exa04 commented May 31, 2024

A single commit? I would just merge this PR

@magnetophon
Copy link
Contributor Author

Most of my PR's have been for nixpkgs, and they trained me into that way of thinking. 😅

@exa04
Copy link
Owner

exa04 commented May 31, 2024

Oh, I see. Well let's get this merged :)

@exa04 exa04 merged commit 83568f0 into exa04:main May 31, 2024
1 of 2 checks passed
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.

2 participants