Skip to content

Conversation

muellmusik
Copy link
Contributor

Adjustments ported from new PD code for VBAP 1.0.3.2. LS amounts allocated dynamically.

@dyfer
Copy link
Member

dyfer commented Jul 2, 2024

Thanks @muellmusik
Could you kindly fix formatting to match the rest of the code? And get rid of the whitespace changes? (These can be viewed in the "Files Changed" tab above)

@dyfer
Copy link
Member

dyfer commented Mar 24, 2025

@muellmusik are you planning to make the requested changes or do you suggest pulling this in as is?

@muellmusik
Copy link
Contributor Author

Sorry @dyfer, I somehow didn't see these.

Could you kindly fix formatting to match the rest of the code? And get rid of the whitespace changes? (These can be viewed in the "Files Changed" tab above)

Could you be more specific as to what you want me to do? The code is mostly from old PD code and is a big mix of tabs and spaces. I can change line 288 and 290 to be one or the other if that's what you mean, but it won't make things consistent so I'm not sure what's best. Also which formatting would you like changed?

@muellmusik are you planning to make the requested changes or do you suggest pulling this in as is?

I'm okay with that personally. It's a little awkward at the moment as I changed machines and haven't managed to set up a build chain yet (Apple dev license woes...) but I can make changes if need. My personal view is that strict standards for formatting have been dispiriting for some new contributors, and thus aren't really worth it, though I know others have differing views.

@dyfer
Copy link
Member

dyfer commented Sep 19, 2025

Could you be more specific as to what you want me to do? The code is mostly from old PD code and is a big mix of tabs and spaces. I can change line 288 and 290 to be one or the other if that's what you mean, but it won't make things consistent so I'm not sure what's best. Also which formatting would you like changed?

I only would like to avoid changes to lines where the only change is whitespace change (spaces<->tabs typically). You can see them in the "File changed" tab of the GH web UI. Example:

Screenshot 2025-09-19 at 2 11 35 PM

By formatting changes I meant changing indentation, but I only really meant that for the lines where this was the only change in the given line, e.g. line 170/174. Sorry I wasn't clear.

I went through the changes and marked where I can see the whitespace change, if you can't see them for some reason. As I wrote elsewhere, I don't want to block this PR based on the whitespace changes, but I would personally prefer not to have them.

My personal view is that strict standards for formatting have been dispiriting for some new contributors, and thus aren't really worth it, though I know others have differing views.

I don't think this request is about any strict standards. I just didn't think that whitespace-only changes are desired. We don't have automatic formatting set up for sc3-plugins so if you're just pushing back on my request to remove whitespace changes, then that's fine, we can merge with the (unnecessary) whitespace changes. This is an unofficial / semi-official repo anyway.

There have been heated discussions in the past on formatting, but I thought that we are past that... If there is an ongoing discussion about formatting as a substantial issue for new contributors, I'm not aware of it.

Comment on lines 145 to 146
}, {table_size = table_size - 1;});
});
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

//"triplet_amount before stripping: %\n".postf(sets.size);
sets = sets.reject({|set|
i1 = set.chanOffsets[0];
i1 = set.chanOffsets[0];
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 233 to 239
+ abs(this.vec_angle(speakers[i], speakers[k]))
+ abs(this.vec_angle(speakers[j], speakers[k])));
+ abs(this.vec_angle(speakers[i], speakers[k]))
+ abs(this.vec_angle(speakers[j], speakers[k])));
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 241 to 249
var length, result;
result = VBAPSpeaker.new;
result.x = (v1.y * v2.z ) - (v1.z * v2.y);
result.y = (v1.z * v2.x ) - (v1.x * v2.z);
var length, result;
result = VBAPSpeaker.new;
result.x = (v1.y * v2.z ) - (v1.z * v2.y);
result.y = (v1.z * v2.x ) - (v1.x * v2.z);
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

(this.vec_length(v1) * this.vec_length(v2));
var inner;
inner = ((v1.x*v2.x) + (v1.y*v2.y) + (v1.z*v2.z)) /
(this.vec_length(v1) * this.vec_length(v2));
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 332 to 338
triplet_amount = sets.size;
//"triplet_amount: %\n".postf(triplet_amount);
triplet_amount = sets.size;
//"triplet_amount: %\n".postf(triplet_amount);
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 342 to 347
lp1 = speakers[set.chanOffsets[0]];
lp1 = speakers[set.chanOffsets[0]];
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 565 to 584
^super.newCopyArgs(chanOffsets);
}
^super.newCopyArgs(chanOffsets);
}
Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 297 to 290
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

whitespace

Comment on lines 722 to 723
final_gs[i]=0.f;
}
Copy link
Member

Choose a reason for hiding this comment

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

whitespace (end of the first line and and beginning of the second line)

Copy link
Member

@mtmccrea mtmccrea left a comment

Choose a reason for hiding this comment

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

If going back to fix white space, I might also recommend clearing out unused variables, which in a few places look to have simply been commented out.

@muellmusik
Copy link
Contributor Author

@dyfer No worries, I'll fix it thanks for marking. I hope this didn't come off as 'heated' in any way. :-)

@muellmusik muellmusik force-pushed the topic/vbap-improvements branch 2 times, most recently from 0503638 to 256b6b9 Compare October 9, 2025 10:58
@muellmusik
Copy link
Contributor Author

Okay @dyfer. I think that's all fixed now. Let me know if there's something more needed. Sorry it took so long.

@muellmusik muellmusik force-pushed the topic/vbap-improvements branch from 256b6b9 to 63fd757 Compare October 9, 2025 11:15
@muellmusik
Copy link
Contributor Author

Oh and removed commented out vars, @mtmccrea

mtmccrea added a commit to ambisonictoolkit/SphericalDesign that referenced this pull request Oct 9, 2025
@dyfer
Copy link
Member

dyfer commented Oct 10, 2025

Thanks @muellmusik
I've run this locally and it seems to work (though I'm not sure if the examples from the help file work as they should... in any case, there doesn't seem to be a change from the previous version).
Speaking of helpfiles, I noticed that the helpfile also mentions the version of the PD code (in multiple places). Could you update that? Or remove the version number from the help altogether?

Adjustments ported from new PD code. LS amounts allocated dynamically.

VBAP 1.0.3.2 Remove ls hardcoded limit

Signed-off-by: Scott Wilson <[email protected]>
@muellmusik muellmusik force-pushed the topic/vbap-improvements branch from 63fd757 to 157f2d8 Compare October 15, 2025 13:59
@muellmusik
Copy link
Contributor Author

Speaking of helpfiles, I noticed that the helpfile also mentions the version of the PD code (in multiple places). Could you update that? Or remove the version number from the help altogether?

Done

@dyfer
Copy link
Member

dyfer commented Oct 15, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Waiting for review response

Development

Successfully merging this pull request may close these issues.

4 participants