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

Removed pow of two table size dependency across all opcodes #1848

Merged
merged 34 commits into from
Apr 30, 2024

Conversation

vlazzarini
Copy link
Member

This PR does three things:

  1. moves floating-point index oscillators poscil etc to ugens2.c
  2. adds truncating floating-point index oscillator code
  3. modifies oscset() to use floating-point index code if a non-power-of-two table is used.

Now oscil, oscili, and oscil3 can be used with any size tables.

@vlazzarini
Copy link
Member Author

vlazzarini commented Mar 22, 2024

I've added fof,fof2,fog now.
I guess I'll complete this by removing the pow2 requirement across the board.
This means changes to the following files, which are using FTFind()

Opcodes/Vosim.c
Opcodes/biquad.c
Opcodes/fareyseq.c
Opcodes/oscbnk.c
Opcodes/partikkel.c
Opcodes/pitch.c
Opcodes/pvadd.c
Opcodes/spectra.c
Opcodes/ugnorman.c
OOps/disprep.c
OOps/ugens1.c
OOps/ugens3.c
OOps/ugens4.c
OOps/ugens4.c

Some work ahead!

@vlazzarini vlazzarini mentioned this pull request Mar 22, 2024
@vlazzarini
Copy link
Member Author

I've dealt with some of these now. Outstanding:

Opcodes/partikkel.c
Opcodes/pitch.c
Opcodes/pvadd.c
Opcodes/spectra.c
Opcodes/ugnorman.c
OOps/disprep.c
OOps/ugens1.c
OOps/ugens3.c
OOps/ugens4.c
OOps/ugens4.c

@vlazzarini
Copy link
Member Author

vlazzarini commented Mar 28, 2024

Outstanding:
OOps/ugens3.c
OOps/ugens4.c

@vlazzarini
Copy link
Member Author

vlazzarini commented Mar 29, 2024

Completed adding floating-point phase to all relevant opcodes and releasing them from pow-of-two dependency.

As a result I have removed the FTFind() function. These module API functions are candidates for consolidation (we should only need 1 function), but there is another PR taking care of it.

There's an outstanding case with vco2init, which generates tables from radix2 FFT processing, thus depending on pow of two size source table. I have an idea on how to modify it to remove that dependency, but I will leave it for a later PR, since this is an edge case.

@vlazzarini vlazzarini changed the title oscil, oscili and oscil3 can use any table size now efficiently Removed pow of two table size dependency across all opcodes Mar 29, 2024
@vlazzarini
Copy link
Member Author

@kunstmusik this is the next one.
I worked fixing the merge from develop and now it should be ready for your review.

H/ugens1.h Show resolved Hide resolved
H/ugens1.h Show resolved Hide resolved
H/ugens3.h Show resolved Hide resolved
H/ugens4.h Show resolved Hide resolved
H/ugens4.h Show resolved Hide resolved
@@ -195,11 +251,28 @@ int32_t foscili(CSOUND *csound, FOSC *p)
v1 = *ftab++;
ar[n] = (v1 + (*ftab - v1) * fract) * amp;
cphs += cinc;
} else {
mphsf = PHMOD1(mphsf);
Copy link
Member

Choose a reason for hiding this comment

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

Awkward indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, forgot to tidy up formatting. Fixed now.

H/csGblMtx.h Outdated
void csoundLock() {
}
void
csoundLock ()
Copy link
Member

Choose a reason for hiding this comment

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

Formatting seems to have taken a step backwards for this PR. Both function definitions and structs got converted to having their opening brace on their own lines, which seems a change from our current conventions.

Copy link
Member Author

@vlazzarini vlazzarini Apr 7, 2024

Choose a reason for hiding this comment

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

do we have a script? Otherwise it is hard to fix? I used clang-format with -style=GNU.

If there is a set format that can be applied, it's an easy fix. If not, we're stuck.

Copy link
Member

Choose a reason for hiding this comment

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

@jpffitch has had a script to format but I don't think it's in the repo? If we go with clang-format it has options we could configure and standardize upon. As it is now, the code formatting differences in recent PRs has definitely increased the difficulty of code reviews. I like the idea of us getting this standardized.

John, could you point to what settings you have used for code formatting? I think we've been using 2-space indent, no tabs, then rest I'm unsure there's been a mix and I think my own preferences (such as always using braces with conditionals) isn't the norm.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now, I reverted the clang-format commit and just corrected the indent of those four files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we merge this now if it's only an issue of formatting?

@kunstmusik
Copy link
Member

I went through as much as I gave up digging into the code, as the formatting has made it near impossible to see actual code changes between develop and this branch. I know we have the code formatting branch to integrate and we should plan to do it carefully; perhaps we should merge all of the other branches we can first, then do the formatting branch and have it be just formatting changes.

Some notes from what I could takeaway:

  • With removing FTFind, FTnp2Find seems a bit awkward of a name to have. We could rename FTnp2Find to FTFind and document the difference.
  • We could be using more of this data type in our code (e.g., for floatph)
  • tempest ⇒ looks like it moved to float phase only?

Compilation worked fine here and some small tests also worked. I'm not 100% confident in my review due to the obscurity in formatting but I think we should proceed with merging and address issues if they appear down the line. Going to go ahead and mere now.

@kunstmusik kunstmusik merged commit 1f4408e into develop Apr 30, 2024
8 checks passed
@vlazzarini
Copy link
Member Author

Thanks.

  • yes the plan is to end up with only one function to find tables, but I stopped short to avoid further merge conflicts.

  • Not sure what you had in mind regarding use of p->floatph, but now that you mentioned it, this could be a property of FUNC

  • yes, tempest makes such limited use of a phase accumulator that I thought we could just simplify it.

Apologies for the formatting issues.

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