Skip to content

Conversation

convextriangle
Copy link
Collaborator

@convextriangle convextriangle commented Jul 22, 2025

Resolves #89;
For double-checking / questions for review stuffs (things that I remembered):

  • are OnsetThreads and FindOnsetIntervals / other old parallel implementations necessary?
  • is it worth it to leave BackgroundThread as a inheritance-ish based std::jthread wrapper or does it need more refactorings (to, e.g., eliminate it)?

@convextriangle convextriangle changed the title replace Thread with C++ concurrency primitives replace Thread.h with C++ concurrency primitives Jul 22, 2025
@convextriangle convextriangle marked this pull request as ready for review July 23, 2025 16:02
@convextriangle convextriangle requested a review from uvcat7 July 23, 2025 16:02
@convextriangle convextriangle linked an issue Jul 28, 2025 that may be closed by this pull request
@sukibaby
Copy link
Collaborator

sukibaby commented Aug 3, 2025

I would appreciate some proofs that this PR maintains expected behavior. I feel like I'm seeing a lot of stuff deleted that I would have expected to be refactored.

@convextriangle
Copy link
Collaborator Author

convextriangle commented Aug 4, 2025

I would appreciate some proofs that this PR maintains expected behavior. I feel like I'm seeing a lot of stuff deleted that I would have expected to be refactored.

  1. parallel execution (iirc) was dead code, uvcat suggested nuking it but i'll check on stuff
  2. proof as in... a video of me trying stuff?

expected to be refactored

  1. such as?
    p.s. i'm asking for clarification - i do have questions as mentioned in the pr and i do not know your expectations as you have not planned/discussed this if i recall correctly so please do @sukibaby

Copy link
Owner

@uvcat7 uvcat7 left a comment

Choose a reason for hiding this comment

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

LGTM. There isn't performance impact on calculating offsets so looks like nothing will revert.

@uvcat7 uvcat7 merged commit 156e20e into uvcat7:beta Aug 9, 2025
5 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.

Replace Thread with std::thread
3 participants