Topic/impulse rewrite #4150

mtmccrea · 2018-11-09T21:06:17Z

Purpose and Motivation

This PR addresses a number of longstanding issues with Impulse that I've aggregated from issues, comments, etc. which are highlighted below.

The primary goal was to initialize the UGen properly and support new frequency and phase argument ranges.

Part fo the initialization sample fix project, specifically in the LFUGens subset.

Types of changes

Added features

Supports positive and negative frequencies.
Phase is wrapped internally, so any phase offset is valid.
Correctly sets the initialization sample and first output sample.

Fixes

#4752, #4075, likely more trigger-related issues

Implementation notes to review

Added a wrapping function.
I chose to use a function to wrap the phase, taking advantage of the fact that the range is [0,1].
This could be useful for other UGens with a phasor in this range.
https://github.com/mtmccrea/supercollider/blob/36cadad0eef499a8122a9136fe2f17acace566ca/server/plugins/LFUGens.cpp#L915-L937
Trigger logic: freq drives impulse triggering, not phase offset.
Phase offset is applied, then phase is wrapped, then phase increment (freq) is applied, then the condition is checked for whether an impulse fires. In this way, phase changes are "instantaneous" and always in-range internally. So phase increment (the frequency control) primarily determines when impulses fire.

In other words, if you advance the phase offset by 3 periods, it's a relative phase offset of 0, so that won't cause an impulse to fire.

Argument input rates aren't limited.
You can read in audio rate arguments even if the UGen running at control rate.
If it's advisable to limit kr ugens to kr arguments, that can be added to the class.
A k-rate frequency argument is now interpolated when running at a-rate.
This wasn't the case before. My understanding is that all k-rate args should be interpolated when used at a-rate, unless there's a particular reason not to.
phOffChanged flag added to Impulse_next_ak.
This flag checks if k-rate phase offset input changes, and if not, skips adding a slope increment of 0 in the sample loop.
Are these kind of optimizations worth it?... benchmarks seem to indicate yes.
https://github.com/mtmccrea/supercollider/blob/36cadad0eef499a8122a9136fe2f17acace566ca/server/plugins/LFUGens.cpp#L963-L973
Aside: we might consider adding support for this kind of optimization in SCUnit::SlopeSignal
Variable names are changed
To more precisely reflect what they are. E.g. unit->mFreq wasn't actually a frequency, but rather a phase increment (unit->mPhaseIncrement).

Bug fixes #

One of the primary issues resolved is the Ctor set the initialization sample to 0, incorrectly setting the initial state of downstream UGens. This bug is expressed in the initial state of, e.g., ToggleFF, RLPF, etc.
Phase offset isn't wrapped - fixed.
Ex: Phase is out of range for 3 k-periods, so erroneously outputs 1s for those 3 control samples.
plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)

{Impulse.kr(10, 3.8)}.plot(duration:0.03).plotMode_(\points);

Negative phase offset isn't handle properly - fixed.
Ex: A phase offset of -1 is equivalent to phase offset of 1, which should trigger an impulse (should output 1 on first sample).

{Impulse.kr(10, -1)}.plot(duration:0.3);

First output sample set incorrectly - fixed.
See Impulse UGEN sets the presample to the wrong value when frequency equals zero #4075

( // envelope triggers one control period late
{
  var sig1 = EnvGen.kr(Env.perc(0.01, 0.04), 1) * SinOsc.ar(440);
  var sig2 = EnvGen.kr(Env.perc(0.01, 0.04), Impulse.kr(0)) * SinOsc.ar(440);
  [sig1,sig2]
}.plot
)

Negative frequencies aren't supported - fixed.
Ex: There's no impulse after first sample:

{Impulse.kr(-10, 0)}.plot(duration:0.3);

Inconsistent behavior with Lag - resolved.
See: plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)
This behaves properly now.

{Lag.ar(Impulse.ar(1, [0, 0.995]), 0.003)}.plot;

Interaction with RLPF - resolved
plugins: Impulse Ctor initial output = 1 for iphase = 0 #2864 (comment)

(
s.waitForBoot({
b = Buffer.alloc(s, 256, 1);
0.3.wait;
a = {
  var sig = RLPF.ar(Impulse.ar(0), 1200, 0.1);
  RecordBuf.ar(sig, b, loop: 0, doneAction: 2);
  sig
}.play;
0.3.wait;
// to inspect the IR
b.getToFloatArray(wait: -1, action: { |data| d = data.postln });
})
)
// before (note impulse is broken also in this signal chain):
// -> FloatArray[ 0.0072281970642507, 0.028581265360117, 0.055974174290895, 0.081284336745739, 0.10381526499987, 0.12296265363693, 0.1382302492857, 0.14924238622189, ...
// after this change, with corrected Impulse:
// -> FloatArray[ 0.035809461027384, 0.084555439651012, 0.13725851476192, 0.18509960174561, 0.22677791118622, 0.26119288802147, 0.28747263550758, 0.30499523878098, ...
// after this change AND after fixing RLPF Ctor:
// -> FloatArray[ 0.0072281970642507, 0.028581265360117, 0.055974174290895, 0.081284336745739, 0.10381526499987, 0.12296265363693, 0.1382302492857, 0.14924238622189...

// see first 100 samples of the result
d[0..99].plot

Note, the original IR in the above comment is "erroneously correct".
This correction to Impulse appeared to "break" this RLPF case, but the problem is to do with an initialization bug in RLPF (#4127), causing the appearance of an impulse double in size and off in phase (coefficients are improperly initialized in RLPF). Once this is corrected, it works again, which coincidentally matches the erroneous IR.

Tests

UPDATE: Newly added unit tests will confirm the above issues are fixed as well as the rate-equivalence tests below.

UnitTest.runTest("TestCoreUGens:test_impulse")

I made numerous tests for the above changes, though here's a basic test comparing the output from all variations of argument rates and output rates:

// test for equal output at different input rates
// Note, this doesn't test modulated parameters, as
// results will vary because of k>a interpolation
(
var funcs;
{
	var frq = -32.9327;
	var phs = -0;
	var rate = \ar; // \ar or \kr

	funcs = [
		{Impulse.perform(rate, DC.ar(frq), DC.ar(phs))},
		{Impulse.perform(rate, DC.ar(frq), DC.kr(phs))},
		{Impulse.perform(rate, DC.ar(frq), phs)},
		{Impulse.perform(rate, DC.kr(frq), DC.ar(phs))},
		{Impulse.perform(rate, DC.kr(frq), DC.kr(phs))},
		{Impulse.perform(rate, DC.kr(frq), phs)},
		{Impulse.perform(rate, frq, DC.ar(phs))},
		{Impulse.perform(rate, frq, DC.kr(phs))},
		{Impulse.perform(rate, frq, phs)},
	];
}.value;

~res = Array.newClear(funcs.size);

fork{
	var cond = Condition();
	funcs.postln;
	funcs.do{ |f, i|
		f.loadToFloatArray(duration: 0.15, action: { |arr| ~res[i] = arr; i.post; " done".postln; cond.test_(true).signal; });
		cond.wait;
		cond.test_(false)
	};
	postf("result: %\n", ~res.every(_ == ~res[0]));
}
)
// inspect one
~res[0].plot.plotMode_(\plines);

Checklist

All previous tests are passing
- 3 tests fail which previously did not (in TestCoreUGens:test_ugen_generator_equivalences), all relating to Trig and Trig1. After applying init sample bug fixes to both UGens, the tests pass.
- A change was made to TestFilterUGens:test_time_invariance to accommodate initialization sample bugs that pervade FilterUGens
Tests were updated or created to address changes in this PR, and tests are passing
Updated documentation, if necessary
This PR is ready for review

mtmccrea · 2019-06-13T17:17:59Z

Update: this PR has been updated with clang-format.

jamshark70

The logic is a massive improvement. Let's please get this into 3.11. There are concrete bugs with this UGen that are fixed here.

I'm going to say "comment" for now rather than "approve" because my comments are questions, not "things to change":

How many calculation functions do we need?
What should be the behavior of a 0-frequency Impulse with a moving phase offset?

jamshark70 · 2020-02-05T05:36:26Z

server/plugins/LFUGens.cpp

+    unit->mPhase = initPhase;
+
+    UnitCalcFunc func;
+    switch (INRATE(0)) {


I don't know how to address it, but when this reaches the stage of c++ style review, I expect there to be a comment about the number of calculation functions. There have been some discussions lately about the topic of handling the permutations.

If you remove all the i functions, then we go from 9 down to 4 -- reducing the maintenance load by a factor of 2.22, while the performance gain of the i functions would be minuscule. So I'd start there.

Yes, 9 is overkill. I've trimmed it to 7 (dropping a-rate phase offset for k- and i-rate frequencies). If needed we could also drop a-rate phase offset altogether. I kept it because I recall a user on the scsynth forums being surprised it wasn't supported.
I agree we could drop i-rate altogether, falling back on a k-rate minimum for both args. Although one of the goals of this PR was support more rates. If maintenance is the issue though, i-rate is the easiest to introspect.

jamshark70 · 2020-02-05T05:43:52Z

server/plugins/LFUGens.cpp

 }

+// Note: In calc functions,


This logic is sound, but I admit, I really had to scratch my head to understand it. Could this comment perhaps be expanded?

A particular sticking point is that it isn't immediately obvious why both phase and phaseOffset are initialized to the phase input. I get it now -- the phase adjustment is based on input phase - previous phase (so, it's 0 if the phase input is constant).

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen. The workaround of course would be HPZ1.ar(Phasor.ar(...)) < 0 instead of Impulse but I guess some users may be surprised by this.

This logic is sound, but I admit, I really had to scratch my head to understand it. Could this comment perhaps be expanded?

Agreed. I've updated the comment to read more clearly.

A particular sticking point is that it isn't immediately obvious why both phase and
phaseOffset are initialized to the phase input.

Part of the confusion may come from the fact that the UGen argument for
phase offset on the sclang side is just called phase. Whereas internally, phase is
the actual phase state value. Throughout the UGen I've renamed local vars
phaseInc and phaseOff to simply inc and off, hopefully disambiguating
it from phase.

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) ...

Great point, I was also surprised the Impulse doesn't support driving by the
phase(offset). Based on your comment I've added this to the Discussion in
the Help file, as well as suggesting the HPZ solution for generating an
impulse train with a Phasor.

muellmusik · 2020-02-06T13:29:40Z

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen.

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME. Don't know if this changes that, but if so it could break code. But maybe that's not an issue here?

jamshark70 · 2020-02-06T13:47:35Z

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME. Don't know if this changes that, but if so it could break code. But maybe that's not an issue here?

I didn't test it (I should -- this is a very important use case!) but I believe it will be fine, based on lines 1023-1025 in the revision:

    if (initPhase == 0.0 && initPhaseInc >= 0.0) {
        initPhase = 1.0; // positive frequency trigger/wrap position
    }

EDIT: though I believe && initPhaseInc >= 0.0 is unnecessary...?

muellmusik · 2020-02-06T14:23:37Z

I didn't test it (I should -- this is a very important use case!) but I believe it will be fine, based on lines 1023-1025 in the revision:

Cool. Sorry if I'm adding noise, just thought it good to check!

mtmccrea · 2020-02-13T09:02:07Z

Thanks @jamshark70 and @muellmusik for having a look at this.
It's been a very long time since I wrote this and I remember it being quite involved and a lot of testing. I'm short on time at the moment but I'd love to move this along. Is there a rough timeline for 3.11?

Briefly,

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g. to excite a resonator) is fairly well established IME.

I do remember testing this, and based on a quick glance at the additions to the help docs, this is a supported case. But, I'd have to rebuild this and test with the case that @jamshark70 brought up:

Users may expect to get triggers from Impulse.ar(0, Phasor.ar(...)) (in the same way that we expect a sine wave from SinOsc.ar(0, Phasor.ar(...))) but in my testing, that doesn't happen.

I'll try to get back into this headspace and test soon. And yes, c++ style review is welcomed! This was a long time ago, before the style guidelines were set, and some of the style here was based on the previous Impulse code, which looked a bit strange.

mossheim · 2020-02-15T22:07:30Z

Is there a rough timeline for 3.11?

3.11 beta should be released on Tuesday; this will probably go into 3.12+.

jamshark70 · 2020-03-01T01:17:26Z

I just (accidentally) discovered some fallout from this:

(
{
	var trig = Impulse.kr(1);
	Sweep.kr(1).poll(trig);
	FreeSelf.kr(PulseCount.kr(trig) >= 3);
	Silent.ar(1)
}.play;
)

// current SC version
UGen(Sweep): 0.00145125
UGen(Sweep): 1.00281
UGen(Sweep): 2.00272

// with this PR: Poll doesn't respond to the initial trigger
// (but, PulseCount *did* catch the first one!)
UGen(Sweep): 1.00281
UGen(Sweep): 2.00272

These changes to Impulse mean that the Ctor initializes the "pre-sample" to 1 when the Impulse is starting at phase 0. I've argued elsewhere that this is correct -- e.g. Lag something that depends on an initial impulse, and in current SC, the lag will start at the wrong value (but it's fixed with this PR): { Lag.ar(Trig1.ar(Impulse.ar(0), 0.002), 0.005) }.plot;.

It also makes sense that inputs expecting a trigger should initialize their unit->m_trig to 0.

But many trigger inputs, e.g. Poll_Ctor, do unit->m_trig = IN0(0);. Now, if you think about it, that's nonsense -- but somehow, it appears in multiple places among the UGens.

So we have two choices:

Revert Impulse's pre-sample to 0. That's the easy way -- but (and I'm willing to argue until I'm blue in the face about it) this is logically incorrect.
Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

(When I raised this initialization issue before, the decision at that time was to avoid breaking existing code. But that has a maintenance cost too, which is now coming due -- if we had fixed it properly back when I brought it up, then this PR would be straightforward.)

mtmccrea · 2020-03-01T22:23:24Z

Thanks @jamshark70 for keeping an eye on this.

It also makes sense that inputs expecting a trigger should initialize their unit->m_trig to 0.

I strongly agree!

So we have two choices:
Revert Impulse's pre-sample to 0. That's the easy way -- but (and I'm willing to argue until I'm blue in the face about it) this is logically incorrect.

I'm strongly against this! I'll get blue in the face about it too ;)

Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

This is the right idea.

I've documented the initialization problem and explained in detail how I think this kind of thing should be resolved:
#4127

geoffroymontel · 2020-05-27T09:45:50Z

I guess this is related to #4976

jamshark70 · 2021-07-05T06:27:25Z

Review other units' trigger inputs initial values. This is more work, but it's really the right way. I think it's better at this point to rip the Band-Aid off and deal with these initial values once and for all.

This is the right idea.

FWIW: I just grepped 'plugins/' for the string trig.* = Z. Almost all occurrences are temporary variables defined within next functions, except:

TRand_Ctor
TExpRand_Ctor
TIRand_Ctor
CoinGate_Ctor

... these 4 include lines like unit->m_trig = ZIN0(2);. By contrast, there are many many more like unit->m_prevtrig = 0.f; -- so the number of fixes may be small.

Also checking for trig.* = I finds only one: Poll (noted above). So, maybe only 5 wrong trigger inits.

mtmccrea · 2022-07-19T17:04:58Z

Back at it again, and hopefully this time for the win :) I've reviewed all comments and updated the OP with clarifications and updates.

^^^ Inline reviews are addressed above ^^^

Some other notable updates:

As requested: trimmed the number of calc functions (still at 7 but could keep cutting...)
renamed variables and made clarifying changes to comments to hopefully make the logic more intelligible
made unit tests for nearly all of the cases on the "bug fixes" list above (as well as updated other unit tests affected by Impulse)

@muellmusik

Perhaps related, the pattern of Impulse.ar(0) producing a single impulse (e.g.
to excite a resonator) is fairly well established IME. Don't know if this
changes that, but if so it could break code.

No problem, this works. :)

@jamshark70

I just (accidentally) discovered some fallout from this: ...
These changes to Impulse mean that the Ctor initializes the "pre-sample" to 1 when the Impulse is starting at phase 0.

This is actually a problem with Poll incorrectly initializing it's trigger state.
After quickly patching Poll's Ctor for the sake of testing, I get closer to the correct
behavior:

UGen(Sweep): 0            // << on initialization
UGen(Sweep): 0.00133333   // << first sample
UGen(Sweep): 1.00267
UGen(Sweep): 2.00267

The first 2 polls happen on the initialization sample (which it ultimately shouldn't), and the second is the first sample. In any case I'm confident Impulse is doing the right thing here.
(Aside: this also reveals is Sweep is likely also not initialized correctly—we should see 0 on the init AND first sample).

FWIW: I just grepped 'plugins/' for the string trig.* = Z. ...

Great, this is a useful approach in addressing the init sample issue. I'll reference this in that thread.

Impulse: remove calc functions for 2 input rate combinations

Impulse_next_ak: consolidate prev_phaseInc and phaseInc vars Throughout: phaseInc -> inc, phaseOff -> off

Update TestFilterUGens:test_time_invariance to accommodate initialization sample bugs that pervade FilterUGens

docs: Impulse: Describe phase offset behavior and supported input rates.

mtmccrea · 2022-08-15T08:01:06Z

Just a ping on this that this PR is ready for final review ☝️

joshpar

This looks good to me! Can you please add documentation for our change log to pull in that describes any breakage from the past, and how to migrate old code in case of breakage? If that is no longer a concern, please let me know and I'll check this to approve.

mtmccrea · 2022-08-23T21:06:08Z

Thanks @joshpar !

Given that this fix could affect the initial state of a number of UGen configurations, we could say something like:

"
Impulse is now initialized correctly such that:

it will fire on the first sample, given the default phase of 0 (or multiple of 1).
a frequency of 0 fires once and only once on the first sample (unless the frequency subsequently changes).
negative frequencies and phases are now supported and phase of any value is wrapped into range.

These are intended and documented behaviors, but which failed in various UGen configurations previously. Therefore, users may observe changes to the initial state of synth graphs that use Impulse. (Especially triggered UGens.)
"

If it's not too verbose, we could also consider providing one concise example of changed/fixed behavior:

( // Previously: EnvGen using Impulse triggered one control period late. Now fixed.
{
  var sig1 = EnvGen.kr(Env.perc(0.01, 0.04), 1) * SinOsc.ar(440);
  var sig2 = EnvGen.kr(Env.perc(0.01, 0.04), Impulse.kr(0)) * SinOsc.ar(440);
  [sig1, sig2]
}.plot
)

... or if that's not appropriate for a changelog, we could just link to this detailed list of resolved behaviors (subsection of this PR, which lists numerous examples): #4150 (comment)

Is that helpful?
Did you want me to put this in the changelog wiki?

joshpar · 2022-08-23T21:24:58Z

@mtmccrea - I think adding it to the changelog wiki is good idea. thanks!

mtmccrea · 2022-08-24T08:42:29Z

@mtmccrea - I think adding it to the changelog wiki is good idea. thanks!

Done! Thanks, @joshpar!

nhthn added this to the 3.11 milestone Nov 15, 2018

nhthn added comp: server plugins API change labels Nov 15, 2018

mtmccrea mentioned this pull request Mar 10, 2019

Impulse UGEN sets the presample to the wrong value when frequency equals zero #4075

Open

mtmccrea force-pushed the topic/impulse-rewrite branch from 36cadad to c76a2ea Compare June 13, 2019 17:16

mtmccrea force-pushed the topic/impulse-rewrite branch from c76a2ea to 7ea859b Compare June 13, 2019 18:54

dyfer mentioned this pull request Feb 4, 2020

Impulse with kr or ar nonzero phase input produces an extra initial impulse #4752

Closed

jamshark70 reviewed Feb 5, 2020

View reviewed changes

dyfer mentioned this pull request Jul 3, 2021

UGens init to unexpected values #2343

Open

mtmccrea added 2 commits July 18, 2022 20:21

plugins: correct Impulse Ctor, add calc functions.

dabf875

docs: Add behavior clarifications to Impulse help file.

1ac5cff

mtmccrea force-pushed the topic/impulse-rewrite branch 2 times, most recently from 151c015 to 14d5d9b Compare July 19, 2022 16:37

mtmccrea mentioned this pull request Jul 20, 2022

Fix UGen Initialization supercollider/rfcs#19

Open

mtmccrea added 5 commits August 12, 2022 10:26

Impulse: reduce number of calc functions

4d65ef2

Impulse: remove calc functions for 2 input rate combinations

Impulse: rename and consolidate variables

bc08f99

Impulse_next_ak: consolidate prev_phaseInc and phaseInc vars Throughout: phaseInc -> inc, phaseOff -> off

Add unit tests for Impulse, update TestFilterUGens

712e45a

Update TestFilterUGens:test_time_invariance to accommodate initialization sample bugs that pervade FilterUGens

docs: Update Impulse help file to indicate supported argument rates.

9a8cc87

docs: Impulse: Describe phase offset behavior and supported input rates.

Impulse: add comment explaining internal logic of Impulse.

fcd97da

mtmccrea force-pushed the topic/impulse-rewrite branch from 14d5d9b to fcd97da Compare August 12, 2022 07:27

dyfer modified the milestones: 3.11, 3.13.0 Aug 15, 2022

joshpar self-assigned this Aug 23, 2022

joshpar requested changes Aug 23, 2022

View reviewed changes

joshpar approved these changes Aug 23, 2022

View reviewed changes

joshpar merged commit 8a5513e into supercollider:develop Aug 23, 2022

mtmccrea mentioned this pull request Feb 9, 2023

Initial state of many UGens is not sample accurate. #4127

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topic/impulse rewrite #4150

Topic/impulse rewrite #4150

mtmccrea commented Nov 9, 2018 •

edited

Loading

mtmccrea commented Jun 13, 2019

jamshark70 left a comment

jamshark70 Feb 5, 2020

mtmccrea Jul 19, 2022

jamshark70 Feb 5, 2020

mtmccrea Jul 19, 2022

muellmusik commented Feb 6, 2020

jamshark70 commented Feb 6, 2020 •

edited

Loading

muellmusik commented Feb 6, 2020

mtmccrea commented Feb 13, 2020

mossheim commented Feb 15, 2020

jamshark70 commented Mar 1, 2020 •

edited

Loading

mtmccrea commented Mar 1, 2020 •

edited

Loading

geoffroymontel commented May 27, 2020

jamshark70 commented Jul 5, 2021 •

edited

Loading

mtmccrea commented Jul 19, 2022 •

edited

Loading

mtmccrea commented Aug 15, 2022

joshpar left a comment

mtmccrea commented Aug 23, 2022 •

edited

Loading

joshpar commented Aug 23, 2022

mtmccrea commented Aug 24, 2022

Topic/impulse rewrite #4150

Topic/impulse rewrite #4150

Conversation

mtmccrea commented Nov 9, 2018 • edited Loading

Purpose and Motivation

Types of changes

Added features

Fixes

Implementation notes to review

Bug fixes #

Tests

Checklist

mtmccrea commented Jun 13, 2019

jamshark70 left a comment

Choose a reason for hiding this comment

jamshark70 Feb 5, 2020

Choose a reason for hiding this comment

mtmccrea Jul 19, 2022

Choose a reason for hiding this comment

jamshark70 Feb 5, 2020

Choose a reason for hiding this comment

mtmccrea Jul 19, 2022

Choose a reason for hiding this comment

muellmusik commented Feb 6, 2020

jamshark70 commented Feb 6, 2020 • edited Loading

muellmusik commented Feb 6, 2020

mtmccrea commented Feb 13, 2020

mossheim commented Feb 15, 2020

jamshark70 commented Mar 1, 2020 • edited Loading

mtmccrea commented Mar 1, 2020 • edited Loading

geoffroymontel commented May 27, 2020

jamshark70 commented Jul 5, 2021 • edited Loading

mtmccrea commented Jul 19, 2022 • edited Loading

mtmccrea commented Aug 15, 2022

joshpar left a comment

Choose a reason for hiding this comment

mtmccrea commented Aug 23, 2022 • edited Loading

joshpar commented Aug 23, 2022

mtmccrea commented Aug 24, 2022

mtmccrea commented Nov 9, 2018 •

edited

Loading

jamshark70 commented Feb 6, 2020 •

edited

Loading

jamshark70 commented Mar 1, 2020 •

edited

Loading

mtmccrea commented Mar 1, 2020 •

edited

Loading

jamshark70 commented Jul 5, 2021 •

edited

Loading

mtmccrea commented Jul 19, 2022 •

edited

Loading

mtmccrea commented Aug 23, 2022 •

edited

Loading