Implementation of Coefficients Components by pasquale90 · Pull Request #1 · Audio-Project-Workgroup/digital_filter_visualiser

pasquale90 · 2026-04-06T21:15:55Z

The main features of this involve:

the implementation of an expandable UI named Coefficients in the left top sub-window, next to the Export option
the implementation of a table that displays the coefficients for the feedforward and the feedback polynomials (ordered by delay), by making use of the RootsToCoefficients::CalculatePolynomialCoefficientsFrom() function
Cells are editable restricting invalid user input
auto-refresh of the coefficient values upon creation/removal/drag of poles/zeros
tooltips on hover for the column names

Missing features:

calculation of roots based on coefficient edits and notification through the ValueTree::Listener
minor stuff mentioned in comments (TODO)

Main features include: Registering to the value tree and updating the table of coefficients based on its updates (creating/deleting/dragging zeros/poles) by passing the roots of the filter to the RootsToCoefficients function Implement editable label fields for allowing users to manually change the values of the coefficients. Updating the value tree based on those edits is missing implementation yet. TODOs are mentioned in code, as a reference of future improvements

… filterState, and call syncListener function to update the coeffTable immediately upon initialization. remove explicit destructor since there shouldn't be memory leaks from dynamic allocations of juce::labels in the refreshComponentForCell. See here: https://docs.juce.com/master/classjuce_1_1TableListBoxModel.html#a873b2b84429f026ea046528af1f4fe81 update comments && clean up

justonem0reuser

Great job, thank you.
Just one thing about RootsToCoefficients that I should have clarified earlier (I will add comments to this class later) is the coefficients order. I suggested the correction for it.

justonem0reuser · 2026-04-08T18:54:23Z

+		processor->filterState->zeros);
+    fbcoeffs = RootsToCoefficients::CalculatePolynomialCoefficientsFrom(
+		processor->filterState->poles);
+


The thing that is not obvious concerning CalculatePolynomialCoefficientsFrom method is that is works in terms of z and we would want to see coefficients in terms of 1/z. That's why we have to reverse the elements order to make the 0-th element be equal to 1.
Probably I should add reversing to CalculatePolynomialCoefficientsFrom method itself to avoid the proposed additional code.

Suggested change

std::reverse(ffcoeffs.begin(), ffcoeffs.end());

std::reverse(fbcoeffs.begin(), fbcoeffs.end());

So doesn't that mean that the "Delay" indexes should be reversed as well, right?

Yes, if the number of zeros is less than the number of poles, then the first several coefficients of the numerator are equal to 0.

Ok, great, but instead of reversing the order as a post-process step, shouldn't that be done in place within the CalculatePolynomialCoefficientsFrom method?

Yes, exactly. I proposed to add reverse to CoeffComponent as a shortest fix for now to merge this PR. And a little bit later I'm going to change CalculatePolynomialCoefficientsFrom method and carefully modify the methods that call it.

To keep things cleaner, afe2210 reverses the order of the coeffs in the CalculatePolynomialCoefficientsFrom method before returning them. So, in case of such a micro-optimization attempt in the future, I don t think there will be a need for modifications outside the scope of this function.

But we cannot just add this to CalculatePolynomialCoefficientsFrom without changing code of each method that calls this method. Otherwise, it introduces bugs in other places.

Oh I see, ok, I have now overwritten the changes here in 8a2410a. Reversing the order is now taking place within CoefficientsComponent::updateCoeffTable function.

A brief comment on this - since this is spanning in other parts of the codebase, in case there s no particular need to reverse the order in the CalculatePolynomialCoefficientsFrom, we can also conform to this implementation without reversing the whole table (cause it now takes 2*O(n)) in each callback, and simply calculate the mirrored indexes to avoid the added overhead. To keep things maintainable, we can keep std::reverse for now and we can discuss this further at one of our next meetings.

justonem0reuser · 2026-04-08T18:56:23Z

+}
+
+void CoefficientsComponent::updateCoeffTable(){
+


Can we remove the next two lines (getting size)?

Sure, yes, thanks for noticing! They are now removed in 5666e50

justonem0reuser · 2026-04-08T21:19:04Z

+        label->onTextChange = [this, row, col, label]{
+            double value = label->getText().getDoubleValue();
+            if (col == 2)
+                ffcoeffs[row] = value;


An exception is thrown here when attempting to change ffcoeffs from 0 to another value and its index is greater or equal than the poles number.

yea, that 's a bug. Thanks for pointing out. And the reason is due to the size mismatch of the containers that hold the numerator and denominator coeffs. In such case we re now filling the rest of the cells in the visualization table with zeros. But these are editable. Therefore an additional check is now added here (9ed7f7c) to ensure that the row index is less than the table size.

…unction

…ting the list of coeffs for visualization purposes(due to a size mismatch between ff and fb), would case segmentation fault when writting back on the source vector

ry-gatoni · 2026-04-15T20:01:24Z

+    tbComp.setStretchToFitActive(true); // expand columns to fill the entire width of the component
+    coeffTable.setVisible(isExpanded);
+
+    processor->filterState->treeRoot.addListener(this);


Suggested change

processor->filterState->treeRoot.addListener(this);

processor->filterState->addListener(this);

Listen to the filter state, not the tree root. This is necessary for loading presets at runtime.

Great, thanks, done! 3f6b78d

ry-gatoni · 2026-04-15T20:08:28Z

+
+void CoefficientsComponent::valueTreePropertyChanged (juce::ValueTree& node, const juce::Identifier& property)
+{
+    if(property == IDs::ValueRe || property == IDs::ValueIm)    // when dragging roots


Suggested change

if(property == IDs::ValueRe || property == IDs::ValueIm) // when dragging roots

if(property == IDs::Order)

{

int order = node.getProperty(IDs::Order);

if(order != 0)

{

updateCoeffTable();

}

}

else if(property == IDs::ValueRe || property == IDs::ValueIm) // when dragging roots

should also update when order changes (unless order is set to zero, in which case update will be handled in valueTreeChildRemoved()).

Yes, that s correct, thanks. Since this is mostly (excluding the case when filter order becomes zero) about later changes in code (that support manual changes of individual root order), and haven t rebased on them yet to maintain the history of the code review, so I 've tested on a local branch and finally updated this PR with (3f6b78d)

…redundant table updates when zeroing down order of roots in the valueTreePropertyChanged method since these are handled in the valueTreeChildRemoved method

…ral fix

… interpretation of the x,width and height values in CoefficientsComponent::resized method

…::CalculatePolynomialCoefficientsFrom method.

justonem0reuser · 2026-04-20T22:45:57Z

I added reversing and setting minimal length to RootsToCoefficients.
I also removed some checks from CoefficientsComponent that I believe are not necessary anymore, but it would be better to review it.
If everything is OK then I think we can merge it.
I decided not to create an additional method and not to add another output value for now, but if we need it then it will be easy to implement.

pasquale90 added 2 commits April 6, 2026 23:55

justonem0reuser requested changes Apr 8, 2026

View reviewed changes

justonem0reuser reviewed Apr 8, 2026

View reviewed changes

pasquale90 added 2 commits April 13, 2026 23:33

remove unnecessary calls of std::vector::size() in updateCoeffTable f…

5666e50

…unction

fix bug where edits of rows containing coefficients that where augmen…

9ed7f7c

…ting the list of coeffs for visualization purposes(due to a size mismatch between ff and fb), would case segmentation fault when writting back on the source vector

ry-gatoni reviewed Apr 15, 2026

View reviewed changes

pasquale90 added 2 commits April 16, 2026 08:07

bind listener to the filter state instead of the tree node and avoid …

3f6b78d

…redundant table updates when zeroing down order of roots in the valueTreePropertyChanged method since these are handled in the valueTreeChildRemoved method

reverse the coefficients' order in updateCoeffTable method as a tempo…

8a2410a

…ral fix

pasquale90 force-pushed the coefComponents branch from afe2210 to 8a2410a Compare April 18, 2026 10:29

pasquale90 and others added 3 commits April 19, 2026 09:58

increased offset from bottom of the Coefficients button, and improved…

9ddd0cb

… interpretation of the x,width and height values in CoefficientsComponent::resized method

implement tooltips on hover of the Coefficient Table headers

1f44a3f

Add setting minimal length and order reversing to RootsToCoefficients…

1beabcc

…::CalculatePolynomialCoefficientsFrom method.


	std::reverse(ffcoeffs.begin(), ffcoeffs.end());
	std::reverse(fbcoeffs.begin(), fbcoeffs.end());

	processor->filterState->treeRoot.addListener(this);
	processor->filterState->addListener(this);

-    if(property == IDs::ValueRe || property == IDs::ValueIm)    // when dragging roots
+    if(property == IDs::Order)
+    {
+        int order = node.getProperty(IDs::Order);
+        if(order != 0)
+        {
+            updateCoeffTable();
+        }
+    }
+    else if(property == IDs::ValueRe || property == IDs::ValueIm)    // when dragging roots

		}

		void CoefficientsComponent::updateCoeffTable(){

Conversation

pasquale90 commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justonem0reuser left a comment

Choose a reason for hiding this comment

Uh oh!

justonem0reuser Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justonem0reuser Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justonem0reuser Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justonem0reuser commented Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pasquale90 commented Apr 6, 2026 •

edited

Loading

justonem0reuser Apr 8, 2026 •

edited

Loading

justonem0reuser Apr 17, 2026 •

edited

Loading

justonem0reuser Apr 8, 2026 •

edited

Loading