Vertical Hold: stop a hold change teleporting the raster

The second half of stoatworks-labs/orrery#6, which came in from an external
user, applied here. Orrery fixed it in 6457f74; the same arithmetic is in this
plugin and in every other one in the fleet that drives a rate off an absolute
clock.

The signal shader rolled the raster with `VerticalHold * Time * 0.65`, so a
change to the control moves the roll by `Time * delta` -- and `Time` is however
long the composition has been open, which an hour in is a jump of hundreds of
fields. Because the roll is wrapped by `fract`, the picture does not slide to
the new rate: it lands at an unrelated vertical offset. It reads as the raster
teleporting rather than as the hold slipping, which is a worse bug than the one
the control is imitating.

So the walk is now accumulated on the host side and handed to the shader as
`VerticalRoll`, a position rather than a product. On a change to the control the
roll reached so far is carried forward and the walk continues from there at the
new rate. Once per change rather than per frame, so nothing accumulates and the
frame rate still cannot move the raster. The anchor starts at time zero at roll
zero, which leaves the value identical to the old product until the control is
first touched -- that is what keeps tools/sweep.py and every rendered-frame
comparison measuring what they measured before.

`VerticalHold` still goes over as well, because the rolling bar's width is an
amplitude and wants the raw value; only the phase use moved.

The OpenFX build mirrors the shader on the CPU, so it gains the same
`verticalRoll` field -- but sets it to the plain `verticalHold * time * 0.65`
product, deliberately. That host renders arbitrary times in arbitrary order and
can keyframe Vertical Hold, so a running carry there would make a frame depend
on which frames happened to be rendered before it. Both sides say so at the
point where the difference would otherwise look like drift between the mirrors.

octest grows --roll, which reads the roll either side of a change rather than
comparing rendered frames: `fract` wraps it, so a jump of a whole number of
fields renders identically and two frames would match for entirely the wrong
reason. It runs ahead of the GL context, so a machine without a GPU can still
run it. Checked against the old arithmetic first: it fails there, and by 2224
fields on the step out of zero.

sweep.py still finds all 34 parameters live, Vertical Hold among them, and
--presets still passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: stoatworks-labs

source/OldCathode.cpp

@@ -261,6 +261,38 @@ void OldCathode::SetClockScaleForTest( double scale )
 	clockScale = scale;
 }
 
+//---------------------------------------------------------------------------
+float OldCathode::VerticalRoll( float seconds )
+{
+	const float hold = params[ PT_VERTICAL_HOLD ];
+
+	// First frame: leave the anchor at time zero, roll zero. That makes this
+	// exactly the old `hold * seconds * 0.65` product for as long as nobody
+	// touches the control, which is what keeps tools/sweep.py and every
+	// rendered-frame comparison measuring the same thing they measured before.
+	if( rollAnchorHold < 0.0f )
+	{
+		rollAnchorHold = hold;
+	}
+	else if( hold != rollAnchorHold )
+	{
+		// Once per change, not once per frame: this carries the exact roll
+		// forward rather than integrating it, so a long session cannot
+		// accumulate rounding into a drift, and the frame rate still cannot
+		// affect where the raster sits.
+		rollAnchor += ( seconds - rollAnchorTime ) * rollAnchorHold * 0.65f;
+		rollAnchorTime = seconds;
+		rollAnchorHold = hold;
+	}
+
+	return rollAnchor + ( seconds - rollAnchorTime ) * hold * 0.65f;
+}
+
+float OldCathode::VerticalRollForTest( float seconds )
+{
+	return VerticalRoll( seconds );
+}
+
 double OldCathode::ClockScaleForTest() const
 {
 	return clockScale;
@@ -436,6 +468,10 @@ FFResult OldCathode::ProcessOpenGL( ProcessOpenGLStruct* pGL )
 		signalShader.Set( "Hum", params[ PT_HUM ] );
 
 		signalShader.Set( "VerticalHold", params[ PT_VERTICAL_HOLD ] );
+		// The anchored walk, not `VerticalHold * Time`: see OldCathode.h. The
+		// control itself still goes over as well, because the rolling bar's
+		// width is an amplitude and wants the raw value.
+		signalShader.Set( "VerticalRoll", VerticalRoll( time ) );
 		signalShader.Set( "Jitter", params[ PT_JITTER ] );
 		signalShader.Set( "Tracking", params[ PT_TRACKING ] );
 		signalShader.Set( "HeadSwitch", params[ PT_HEAD_SWITCH ] );

source/OldCathode.h

@@ -179,6 +179,14 @@ class OldCathode : public CFFGLPlugin
 	void SetClockScaleForTest( double scale );
 	double ClockScaleForTest() const;
 
+	/// How far the raster has walked at a given moment, in fractions of a field.
+	/// `--roll` needs it: the thing being tested is that a Vertical Hold change
+	/// does NOT jump the picture, and reading the roll either side of one says
+	/// so directly -- where comparing rendered frames could not, because the
+	/// roll is wrapped into 0..1 and a jump of a whole number of fields renders
+	/// identically.
+	float VerticalRollForTest( float seconds );
+
 private:
 
 
@@ -198,6 +206,31 @@ class OldCathode : public CFFGLPlugin
 	int phosphorIndex = 0;  //!< Which half of the ping-pong this frame writes to.
 	float frameIndex = 0.0f;//!< Drives the subcarrier's frame-to-frame phase walk.
 
+	//---------------------------------------------------------------------
+	// Where the raster has walked to.
+	//
+	// The picture stays a pure function of the roll -- what changes here is
+	// only which roll a given clock reading maps to.
+	//
+	// `roll = verticalHold * time * 0.65` means a Vertical Hold change moves the
+	// roll by `time * delta`, and `time` is however long the composition has
+	// been open. Nudging the control an hour in is a jump of hundreds of fields,
+	// and because the roll wraps the picture lands at an unrelated offset: it
+	// reads as the raster teleporting rather than as the hold slipping. That is
+	// the same defect orrery issue #6 reported for its Speed control. So
+	// remember the roll reached so far and carry on from there at the new rate.
+	//
+	// Not in the OpenFX build, which renders arbitrary times in arbitrary order
+	// and can keyframe Vertical Hold: a running carry there would make a frame
+	// depend on which frames were rendered before it. That build keeps the pure
+	// product, and says so.
+	//---------------------------------------------------------------------
+	float VerticalRoll( float seconds );
+
+	float rollAnchor  = 0.0f;///< roll already reached at `rollAnchorTime`
+	float rollAnchorTime = 0.0f;///< the clock reading that roll belongs to
+	float rollAnchorHold = -1.0f;///< hold in force since then; < 0 until the first frame
+
 	double clockScale   = 0.0;///< 0 until decided; then 1.0 or 0.001
 	double lastRawTime  = -1.0;
 	double lastWallTime = -1.0;

source/ofx/OldCathodeOFX.cpp

@@ -193,6 +193,15 @@ struct SignalSettings
 
 	float noise = 0, dropouts = 0, ghostAmount = 0, ghostDelay = 0, interference = 0, hum = 0;
 	float verticalHold = 0, jitter = 0, tracking = 0, headSwitch = 0;
+
+	/// How far the raster has walked, in fractions of a field: the shader's
+	/// `VerticalRoll` uniform. The FFGL build anchors this so that nudging
+	/// Vertical Hold live does not teleport the picture; here it stays the plain
+	/// `verticalHold * time * 0.65` product, because this host renders arbitrary
+	/// times in arbitrary order and can keyframe the control -- a running carry
+	/// would make a frame depend on which frames happened to be rendered before
+	/// it. See OldCathode.h.
+	float verticalRoll = 0;
 	bool interlace = false;
 
 	float time       = 0.0f;
@@ -318,7 +327,7 @@ class SignalStage
 	{
 		const float texelY = 1.0f / st.signalH;
 
-		const float srcY    = fractf( v + st.verticalHold * st.time * 0.65f );
+		const float srcY    = fractf( v + st.verticalRoll );
 		const float lineIdx = std::floor( srcY * st.signalH );
 		const float lineRnd = rnd( lineIdx, st.frameIndex, 5.0f ) - 0.5f;
 
@@ -1100,6 +1109,7 @@ class OldCathodePlugin : public OFX::ImageEffect
 		sig.hum          = float( hum->getValueAtTime( t ) );
 
 		sig.verticalHold = float( verticalHold->getValueAtTime( t ) );
+		sig.verticalRoll = 0.0f;//per-frame, below: it depends on the frame's own time
 		sig.jitter       = float( jitter->getValueAtTime( t ) );
 		sig.tracking     = float( tracking->getValueAtTime( t ) );
 		sig.headSwitch   = float( headSwitch->getValueAtTime( t ) );
@@ -1112,6 +1122,7 @@ class OldCathodePlugin : public OFX::ImageEffect
 
 			SignalSettings frameSig = sig;
 			frameSig.time           = float( frameTime / fps );
+			frameSig.verticalRoll   = frameSig.verticalHold * frameSig.time * 0.65f;
 			frameSig.frameIndex     = float( std::fmod( frameTime, 100000.0 ) );
 
 			SignalStage stage( sd, frameSig );

source/shaders/Signal.cpp

@@ -61,6 +61,12 @@ uniform float Hum;
 
 //Timebase
 uniform float VerticalHold;
+
+//How far the raster has walked, in fractions of a field. NOT `VerticalHold *
+//Time`: that is an absolute product, so moving the control an hour into a
+//composition jumps the picture by hundreds of fields at once. The host side
+//anchors it -- see OldCathode.h -- and hands over the position it has reached.
+uniform float VerticalRoll;
 uniform float Jitter;
 uniform float Tracking;
 uniform float HeadSwitch;
@@ -256,7 +262,7 @@ void main()
 
 	//Vertical hold: the field no longer starts where the flyback expects it to,
 	//so the whole raster walks and takes the blanking interval with it.
-	float srcY = fract( uv.y + VerticalHold * Time * 0.65 );
+	float srcY = fract( uv.y + VerticalRoll );
 
 	float lineIdx = floor( srcY * SignalSize.y );
 	float lineRnd = rnd( lineIdx, FrameIndex, 5.0 ) - 0.5;

tools/octest/main.cpp

@@ -335,6 +335,86 @@ bool readExactly( void* into, size_t bytes )
 	return true;
 }
 
+//---------------------------------------------------------------------------
+/// Prove a Vertical Hold change does not jump the picture.
+///
+/// The roll either side of the change is read directly rather than comparing
+/// rendered frames: the roll is wrapped into 0..1 by `fract`, so a jump of a
+/// whole number of fields renders identically and two frames would match for
+/// entirely the wrong reason. The number says it outright.
+///
+/// Needs no GL, so it runs ahead of the context.
+//---------------------------------------------------------------------------
+int runRollTest()
+{
+	int failures = 0;
+
+	auto check = [ &failures ]( const char* what, double got, double want, double tol ) {
+		const bool ok = std::fabs( got - want ) <= tol;
+		std::printf( "roll %-40s got=%-14.6f want=%-14.6f %s\n", what, got, want, ok ? "ok" : "FAILED" );
+		if( !ok )
+			++failures;
+	};
+
+	const float holds[] = { 0.10f, 0.95f, 0.00f, 0.40f };
+
+	OldCathode plugin;
+	plugin.SetClockScaleForTest( 1.0 );//seconds, said out loud rather than inferred
+
+	// By display name, as everything else in this harness does: the PT_ enum is
+	// private to the plugin and the name is the host's own handle on the control.
+	unsigned int holdIndex = 0;
+	bool found             = false;
+	for( unsigned int i = 0; i < plugin.GetNumParams(); ++i )
+	{
+		const char* name = plugin.GetParamName( i );
+		if( name != nullptr && std::string( name ) == "Vertical Hold" )
+		{
+			holdIndex = i;
+			found     = true;
+			break;
+		}
+	}
+	if( !found )
+	{
+		std::fprintf( stderr, "roll: no parameter named 'Vertical Hold'\n" );
+		return 1;
+	}
+
+	// An hour in, which is where the old arithmetic hurt most and where a live
+	// operator actually is when they reach for the control. The clock only ever
+	// goes forwards from here.
+	float seconds = 3600.0f;
+
+	// Untouched, the anchor must leave the old expression exactly as it was --
+	// this is what keeps tools/sweep.py and every rendered-frame comparison
+	// measuring the same thing they measured before. Vertical Hold defaults to
+	// zero, so this is zero, and the step below out of zero is the one that
+	// used to hurt most.
+	check( "untouched == hold * seconds * 0.65", plugin.VerticalRollForTest( seconds ),
+	       plugin.GetFloatParameter( holdIndex ) * seconds * 0.65, 1e-3 );
+
+	for( const float hold : holds )
+	{
+		const float before = plugin.VerticalRollForTest( seconds );
+
+		// The same instant, a new hold: nothing about the clock has moved, so
+		// the raster may not move either.
+		plugin.SetFloatParameter( holdIndex, hold );
+		check( "a Vertical Hold change does not jump the raster",
+		       plugin.VerticalRollForTest( seconds ), before, 1e-3 );
+
+		// And then it must actually walk at the new rate.
+		const float resumed = plugin.VerticalRollForTest( seconds );
+		seconds += 1.0f;
+		check( "  and walks at the new rate afterwards",
+		       plugin.VerticalRollForTest( seconds ) - resumed, hold * 0.65, 1e-3 );
+	}
+
+	std::printf( "%s\n", failures == 0 ? "roll: all ok" : "roll: FAILURES" );
+	return failures == 0 ? 0 : 1;
+}
+
 void usage()
 {
 	std::printf(
@@ -350,6 +430,7 @@ void usage()
 		"  --measure         print the mean RGB of the middle of the picture\n"
 		"  --list            print every parameter and its default, then exit\n"
 		"  --presets         every factory preset survives every host behaviour\n"
+		"  --roll            a Vertical Hold change does not jump the picture\n"
 		"\n"
 		"  --pipe            read raw RGBA frames from stdin, write them to stdout,\n"
 		"                    so real footage can be put through the chain:\n"
@@ -509,6 +590,7 @@ int main( int argc, char** argv )
 	bool keepAlpha = false;
 	bool listOnly = false;
 	bool measure = false;
+	bool rollOnly = false;
 	bool pipeMode = false;
 	float flatLevel = -1.0f;
 	std::string scriptPath;
@@ -522,6 +604,8 @@ int main( int argc, char** argv )
 
 		if( arg == "--out" )
 			outputPath = next();
+		else if( arg == "--roll" )
+			rollOnly = true;
 		else if( arg == "--width" )
 			width = std::atoi( next().c_str() );
 		else if( arg == "--height" )
@@ -590,6 +674,11 @@ int main( int argc, char** argv )
 		return -1;
 	};
 
+	// Ahead of the GL context on purpose: this one needs no GPU, so it still
+	// runs on a machine that cannot make a context at all.
+	if( rollOnly )
+		return runRollTest();
+
 	if( listOnly )
 	{
 		for( unsigned int i = 0; i < plugin.GetNumParams(); ++i )