Skip to content

Commit 228920d

Browse files
Presets: stop the host's own echo dropping them back to Custom
The same bug an external user reported against vertigo as its issue #2, which all seven plugins carried because the preset machinery was copied between them. Choosing a factory preset in Resolume did nothing: the dropdown snapped straight back to Custom. applyPreset wrote its values into params[] and raised FF_EVENT_FLAG_VALUE so the host would re-read its sliders. That rests on an assumption FFGL never makes. The host owns parameter state; it pushes its own values back down whenever it likes, and nothing obliges it to act on a value event. Resolume does not act on them -- it carries on restating the values it still believes in, the ones from before the preset. Those restatements arrive as SetFloatParameter calls carrying a changed value, so the rule that a covered parameter changing means the operator has taken over fired on the host's own echo, immediately, every time. Three things now arrive through that one call while a preset is active and only the third is a person, so they are told apart by what the value IS rather than by the fact that it changed: matching the preset is the host agreeing with us, matching hostValues[] -- the host's own last word, kept separately from what the plugin renders with -- is the host restating itself, and anything else is an edit. The first two are ignored rather than written. Ignoring rather than writing matters for the first as well as the second: a host that quantises hands back a ROUNDED copy of our own value, and the pre-existing "did a covered parameter move?" test works to a tighter tolerance than this one and would read that rounding as an edit. seedHostValues() fills the record from the defaults on first parameter traffic, which has to happen before applyPreset can run -- seeding afterwards records the preset's own values as the host's opening position and the very next restatement looks like an edit. That mistake was made once while writing this and the test caught it. Dropping to Custom is now logged with the parameter and value that caused it. Diagnosing the original report needed a code read for exactly the reason orrery issue #6 did: nothing said it had happened. The harness grows --presets, which drives three hosts -- one that honours value events, one that ignores them, one that honours them but quantises to 1/1000 -- across every preset, with no GL involved. Against the pre-fix code it fails in precisely the "ignores" column, which is the shape of the bug as reported. sweep.py also learns to stop at the About block, as porthole's already had: those are browser buttons that never touch a pixel, and sweeping them reported five dead controls and buried anything real. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 1834399 commit 228920d

5 files changed

Lines changed: 313 additions & 0 deletions

File tree

AGENTS.md

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,49 @@ still the only thing that measures anything.
313313

314314
## Factory presets
315315

316+
### The host owns the parameters, and a preset had to learn that
317+
318+
Reported against **vertigo** as its issue #2 and fixed across all seven plugins
319+
on 2026-08-22: choosing a factory preset in Resolume did nothing and the
320+
dropdown snapped straight back to `Custom`.
321+
322+
The pattern was copy-based — `applyPreset` writes the values into `params[]` and
323+
raises `FF_EVENT_FLAG_VALUE` so the host re-reads its sliders — and it rests on
324+
an assumption FFGL never makes. **The host owns parameter state.** It pushes its
325+
own values back down whenever it likes, and nothing obliges it to act on a value
326+
event. Resolume does not: it carries on restating the values it still believes
327+
in, which are the ones from before the preset. Those restatements arrive as
328+
`SetFloatParameter` calls carrying a changed value, so the rule "a covered
329+
parameter changed, therefore the operator has taken over" fired on the host's
330+
own echo, instantly, every time.
331+
332+
Three things now arrive through that one call while a preset is active, and only
333+
the third is a person:
334+
335+
| What arrives | How it is recognised | What happens |
336+
|---|---|---|
337+
| the preset's own values, from a host that honoured the events | matches the preset | ignored — nothing to write |
338+
| the values from *before* the preset, from a host that did not | matches `hostValues[]`, the host's own last word | ignored — writing it would undo the preset |
339+
| a new value from neither | matches neither | written, and the preset falls back to Custom |
340+
341+
`hostValues[]` is the record of what the **host** last sent, which is not what
342+
the plugin is rendering with, and `seedHostValues()` fills it from the defaults
343+
on the first parameter traffic — **before `applyPreset` can run**. Seeding it
344+
afterwards records the preset's own values as the host's opening position, so
345+
the host's very next restatement looks like an edit; that mistake was made once
346+
during the fix and the test caught it.
347+
348+
Two tolerances matter and they are not the same number. `kSame` is **1e-3**, a
349+
host-quantisation allowance rather than a float epsilon — a host that keeps its
350+
parameters shorter than a float hands back a number *near* ours. The pre-existing
351+
"did a covered parameter move?" test below still works to 1e-4, which is why a
352+
value matching the preset is **ignored rather than written**: letting a rounded
353+
copy of our own value into `params[]` would trip that tighter test.
354+
355+
`octest --presets` drives all three hosts across every preset, with no GL
356+
involved, and runs in `tools/verify.sh`. Against the pre-fix code it fails in
357+
exactly the "ignores value events" column.
358+
316359
`source/Presets.h` is one table of named looks in the host-facing 0..1
317360
parameter space, and it drives **both** builds — the FFGL constructor and the
318361
OFX describe each read it, so a preset cannot drift between Resolume and

source/OldCathode.cpp

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,8 @@ FFResult OldCathode::SetFloatParameter( unsigned int index, float value )
620620
if( index >= PT_COUNT )
621621
return FF_FAIL;
622622

623+
seedHostValues();
624+
623625
// An About button is a press, not a value to keep: it opens a browser and
624626
// nothing about the effect changes.
625627
if( index >= PT_ABOUT_FIRST )
@@ -633,6 +635,14 @@ FFResult OldCathode::SetFloatParameter( unsigned int index, float value )
633635
return FF_SUCCESS;
634636
}
635637

638+
// The host may be restating a value it still believes in rather than the
639+
// operator moving anything. Letting that through would overwrite the
640+
// preset's value in params[] AND read as an edit, dropping the dropdown
641+
// back to Custom -- which is what made presets look like they could not
642+
// be selected at all. See AGENTS.md.
643+
if( hostIsRestatingItself( index, value ) )
644+
return FF_SUCCESS;
645+
636646
// A slider moved while a preset is active means the operator has taken
637647
// over: the dropdown falls back to Custom. The equality guard matters —
638648
// hosts that honour the value events echo the preset's own values straight
@@ -647,6 +657,13 @@ FFResult OldCathode::SetFloatParameter( unsigned int index, float value )
647657
{
648658
if( id == index )
649659
{
660+
// Logged, unlike an ordinary parameter change: this one is a
661+
// state change an operator can be surprised by, it happens once
662+
// rather than per frame, and diagnosing vertigo #2 needed a code
663+
// read precisely because nothing said it had happened.
664+
diag::info( "preset dropped to Custom: parameter "
665+
+ std::to_string( index ) + " moved to "
666+
+ std::to_string( value ) );
650667
params[ PT_PRESET ] = 0.0f;
651668
RaiseParamEvent( PT_PRESET, FF_EVENT_FLAG_VALUE );
652669
break;
@@ -657,6 +674,76 @@ FFResult OldCathode::SetFloatParameter( unsigned int index, float value )
657674
return FF_SUCCESS;
658675
}
659676

677+
const unsigned int* OldCathode::PresetParamIDsForTest( int& count )
678+
{
679+
count = oldcathode::presets::kParamCount;
680+
return kPresetParamIDs;
681+
}
682+
683+
float OldCathode::presetValue( int presetIndex, unsigned int id ) const
684+
{
685+
if( presetIndex <= 0 || presetIndex > oldcathode::presets::kCount )
686+
return -1.0f;
687+
688+
const oldcathode::presets::Preset& preset = oldcathode::presets::kPresets[ presetIndex - 1 ];
689+
for( int j = 0; j < oldcathode::presets::kParamCount; ++j )
690+
if( kPresetParamIDs[ j ] == id )
691+
return preset.v[ j ];
692+
693+
return -1.0f;
694+
}
695+
696+
void OldCathode::seedHostValues()
697+
{
698+
// Seeded on first parameter traffic rather than in the constructor, so the
699+
// whole mechanism stays in one place. It has to happen BEFORE applyPreset
700+
// can run: seeding afterwards would record the preset's own values as the
701+
// host's opening position, and the host's very next restatement would then
702+
// look like an edit -- which is the bug this exists to fix, reintroduced.
703+
if( hostValuesSeeded )
704+
return;
705+
706+
for( unsigned int i = 0; i < PT_COUNT; ++i )
707+
hostValues[ i ] = params[ i ];
708+
hostValuesSeeded = true;
709+
}
710+
711+
bool OldCathode::hostIsRestatingItself( unsigned int index, float value )
712+
{
713+
const float lastFromHost = hostValues[ index ];
714+
hostValues[ index ] = value;
715+
716+
const float fromPreset =
717+
presetValue( static_cast< int >( std::lround( params[ PT_PRESET ] ) ), index );
718+
if( fromPreset < 0.0f )
719+
return false;
720+
721+
// A quantisation allowance rather than a float epsilon. A host that keeps
722+
// its parameters shorter than a float -- or round-trips them through a UI,
723+
// a MIDI value or a saved composition -- hands back a number near ours
724+
// rather than ours, and 1e-4 read that as an edit.
725+
constexpr float kSame = 1e-3f;
726+
727+
if( std::fabs( value - fromPreset ) <= kSame )
728+
{
729+
// The host agreeing with the preset. Nothing to write -- and writing it
730+
// would actively hurt: a host that quantises hands back a ROUNDED copy
731+
// of our own value, params[] would take the rounding, and the existing
732+
// "did a covered parameter move?" test below works to a tighter
733+
// tolerance than this one and would read that rounding as an edit.
734+
return true;
735+
}
736+
737+
if( std::fabs( value - lastFromHost ) > kSame )
738+
return false;//neither: the operator has taken over
739+
740+
// Deliberately not logged. A host that pushes its parameters every frame
741+
// would put a line here every frame, and a log that scrolls is a log nobody
742+
// reads. The event worth recording is the one below, in the fallback to
743+
// Custom, which happens once.
744+
return true;
745+
}
746+
660747
void OldCathode::applyPreset( int presetIndex )
661748
{
662749
params[ PT_PRESET ] = static_cast< float >( presetIndex );

source/OldCathode.h

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,11 @@ class OldCathode : public CFFGLPlugin
5252
FFResult SetTextParameter( unsigned int index, const char* value ) override;
5353
float GetFloatParameter( unsigned int index ) override;
5454

55+
/// Test hook: the parameter ids a preset covers, in presets::Param
56+
/// order. Handed out rather than copied into the harness, so a second
57+
/// list cannot go quietly out of step with this one.
58+
static const unsigned int* PresetParamIDsForTest( int& count );
59+
5560
FFResult SetTime( double time ) override;
5661

5762
private:
@@ -125,8 +130,39 @@ class OldCathode : public CFFGLPlugin
125130

126131
/// Copy a factory preset's values into params[] and raise value events so
127132
/// the host re-reads the sliders. `presetIndex` is 1-based; 0 is Custom.
133+
/// The active preset's value for `id`, or -1 when no preset is active or
134+
/// this one has no opinion about `id`. Preset values are all 0..1, so a
135+
/// negative is unambiguous.
136+
float presetValue( int presetIndex, unsigned int id ) const;
137+
138+
/// True when this write is the HOST restating a value it still believes in
139+
/// rather than the operator moving anything -- in which case it must not
140+
/// reach params[] and must not disturb the preset.
141+
bool hostIsRestatingItself( unsigned int index, float value );
142+
143+
/// Record the defaults as the host's opening position, once, before
144+
/// anything has had a chance to move them.
145+
void seedHostValues();
146+
128147
void applyPreset( int presetIndex );
129148

149+
/// What the HOST last sent for each parameter, which is not the same thing
150+
/// as what the plugin is rendering with.
151+
///
152+
/// FFGL's host owns parameter state. It pushes its own values back down
153+
/// whenever it likes, and nothing obliges it to act on the value events
154+
/// applyPreset raises -- Resolume does not. So a preset that writes params[]
155+
/// and trusts the host to follow is relying on behaviour the specification
156+
/// never promised, and when the host instead restates the values it still
157+
/// believes in, the rule that a covered parameter changing means the
158+
/// operator has taken over fires on the host's own echo and drops straight
159+
/// back to Custom. Reported against vertigo as its issue #2; the same
160+
/// pattern had been copied into all seven plugins.
161+
///
162+
/// Keeping the host's own last word separately is what tells the two apart.
163+
float hostValues[ PT_COUNT ] = {};
164+
bool hostValuesSeeded = false;
165+
130166
bool compileShaders();
131167
void releaseBuffers();
132168

tools/octest/main.cpp

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,7 @@ void usage()
349349
" --flat V render a uniform field at level V instead of the test card\n"
350350
" --measure print the mean RGB of the middle of the picture\n"
351351
" --list print every parameter and its default, then exit\n"
352+
" --presets every factory preset survives every host behaviour\n"
352353
"\n"
353354
" --pipe read raw RGBA frames from stdin, write them to stdout,\n"
354355
" so real footage can be put through the chain:\n"
@@ -360,6 +361,145 @@ void usage()
360361
}
361362
} // namespace
362363

364+
//---------------------------------------------------------------------------
365+
/// Prove a factory preset survives whatever the host does next.
366+
///
367+
/// FFGL's host owns parameter state and is free to push it back down at any
368+
/// time, and nothing in the specification obliges it to act on the value
369+
/// events a plugin raises when it changes a parameter itself. So there are
370+
/// three hosts to survive, and the plugin cannot tell which one it is talking
371+
/// to:
372+
///
373+
/// - one that honours the events and hands the new values straight back;
374+
/// - one that ignores them and carries on restating the values it still
375+
/// believes in, which are the ones from before the preset;
376+
/// - one that honours them but keeps its parameters shorter than a float, so
377+
/// what comes back is near the preset rather than equal to it.
378+
///
379+
/// All three arrive as SetFloatParameter calls carrying a changed value, which
380+
/// is why "the value changed, so the operator must have taken over" is the
381+
/// wrong test. Resolume is the second kind, and against the unfixed code this
382+
/// fails in exactly that column -- reported as vertigo issue #2.
383+
///
384+
/// No GL here: this is the parameter plumbing, not the picture.
385+
//---------------------------------------------------------------------------
386+
int runPresetTest()
387+
{
388+
using namespace oldcathode::presets;
389+
390+
int coveredCount = 0;
391+
const unsigned int* covered = OldCathode::PresetParamIDsForTest( coveredCount );
392+
393+
enum class Host
394+
{
395+
Honours,
396+
Ignores,
397+
Quantises
398+
};
399+
struct HostCase
400+
{
401+
Host kind;
402+
const char* name;
403+
};
404+
const HostCase hosts[] = {
405+
{ Host::Honours, "honours value events" },
406+
{ Host::Ignores, "ignores value events" },
407+
{ Host::Quantises, "honours, 1/1000 steps" },
408+
};
409+
410+
int failures = 0;
411+
412+
for( const HostCase& host : hosts )
413+
{
414+
for( int preset = 1; preset <= kCount; ++preset )
415+
{
416+
OldCathode plugin;
417+
418+
int presetIndex = -1;
419+
for( unsigned int i = 0; i < plugin.GetNumParams(); ++i )
420+
{
421+
const char* declared = plugin.GetParamName( i );
422+
if( declared != nullptr && std::strcmp( declared, "Preset" ) == 0 )
423+
{
424+
presetIndex = int( i );
425+
break;
426+
}
427+
}
428+
if( presetIndex < 0 )
429+
{
430+
std::fprintf( stderr, "presets: no parameter is called \"Preset\"\n" );
431+
return 1;
432+
}
433+
434+
// What the host thinks the sliders say before the operator reaches
435+
// for the dropdown.
436+
std::vector< float > hostOwn;
437+
for( int j = 0; j < coveredCount; ++j )
438+
hostOwn.push_back( plugin.GetFloatParameter( covered[ j ] ) );
439+
440+
// The operator picks a preset.
441+
plugin.SetFloatParameter( unsigned( presetIndex ), float( preset ) );
442+
443+
// And now the host says its piece.
444+
for( int j = 0; j < coveredCount; ++j )
445+
{
446+
float back = 0.0f;
447+
switch( host.kind )
448+
{
449+
case Host::Honours:
450+
back = plugin.GetFloatParameter( covered[ j ] );
451+
break;
452+
case Host::Ignores:
453+
back = hostOwn[ size_t( j ) ];
454+
break;
455+
case Host::Quantises:
456+
back = std::round( plugin.GetFloatParameter( covered[ j ] ) * 1000.0f ) / 1000.0f;
457+
break;
458+
}
459+
plugin.SetFloatParameter( covered[ j ], back );
460+
}
461+
462+
const int still = int( std::lround( plugin.GetFloatParameter( unsigned( presetIndex ) ) ) );
463+
bool ok = still == preset;
464+
465+
// Still selected is not enough -- it has to be what renders.
466+
for( int j = 0; j < coveredCount; ++j )
467+
{
468+
const float want = kPresets[ preset - 1 ].v[ j ];
469+
const float got = plugin.GetFloatParameter( covered[ j ] );
470+
ok = ok && std::fabs( got - want ) <= 1e-4f;
471+
}
472+
473+
if( !ok )
474+
{
475+
std::printf( "presets %-22s %-22s FAILED (shows %d)\n",
476+
host.name, kPresets[ preset - 1 ].name, still );
477+
++failures;
478+
continue;
479+
}
480+
481+
// An operator turning a covered knob must still drop to Custom -- a
482+
// preset that cannot be left is no better than one that will not
483+
// stick. Move it somewhere neither the preset nor the host named.
484+
const float moved = kPresets[ preset - 1 ].v[ 0 ] > 0.5f ? 0.123f : 0.877f;
485+
plugin.SetFloatParameter( covered[ 0 ], moved );
486+
const int after = int( std::lround( plugin.GetFloatParameter( unsigned( presetIndex ) ) ) );
487+
if( after != 0 )
488+
{
489+
std::printf( "presets %-22s %-22s FAILED (an edit left it on %d)\n",
490+
host.name, kPresets[ preset - 1 ].name, after );
491+
++failures;
492+
continue;
493+
}
494+
495+
std::printf( "presets %-22s %-22s ok\n", host.name, kPresets[ preset - 1 ].name );
496+
}
497+
}
498+
499+
std::printf( "%s\n", failures == 0 ? "presets: all ok" : "presets: FAILURES" );
500+
return failures == 0 ? 0 : 1;
501+
}
502+
363503
int main( int argc, char** argv )
364504
{
365505
std::string outputPath = "/tmp/oldcathode.png";
@@ -400,6 +540,8 @@ int main( int argc, char** argv )
400540
fps = std::strtod( next().c_str(), nullptr );
401541
else if( arg == "--flat" )
402542
flatLevel = std::strtof( next().c_str(), nullptr );
543+
else if( arg == "--presets" )
544+
return runPresetTest();
403545
else if( arg == "--list" )
404546
listOnly = true;
405547
else if( arg == "--set" )

tools/sweep.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,11 @@ def diff(a, b):
7272
names = subprocess.run(["./build/octest", "--list"], capture_output=True, text=True).stdout
7373
params = [' '.join(l.split()[1:-1]) for l in names.strip().splitlines()]
7474

75+
# The About block is a text field and browser buttons, declared last. They
76+
# never touch a pixel, so sweeping them only buries a real dead control.
77+
if "About" in params:
78+
params = params[:params.index("About")]
79+
7580
# Options are discrete; sweep them across their real element range.
7681
DISCRETE = {"System": (0, 1), "Source": (0, 3), "Mask Pattern": (1, 4), "Interlace": (0, 1)}
7782

0 commit comments

Comments
 (0)