Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for multiple Rocket clients #165

Merged
merged 4 commits into from
Aug 17, 2023

Conversation

vcaputo
Copy link
Contributor

@vcaputo vcaputo commented Feb 25, 2023

This adds the ability for RocketEditor to accept multiple
simultaneous Rocket connections.

The connection which performs GET_TRACK is assumed to be the
Demo, which is a privileged client owning the map of track
names<->IDs. If another client attempts to GET_TRACK or SET_KEY
than the current Demo client, that client is diconnected and an
error logged. This single-demo limitation greatly simplifies
supporting multiple clients, as it skirts trying to turn the
track-ID mapping into more of a distributed mapping within the
existing Rocket protocol.

All clients however are permitted to SET_ROW and PAUSE, with
these commands also being broadcast to all clients for keeping
them synchronized.

There's some ickiness to handling SET_ROW propagation during
playback, since it's unlikely the clients are all perfectly
aligned on their current row.

So when the editor is unpaused, any incoming SET_ROW isn't
broadcast unless it differs by >= 1-second worth of rows. This
is done to prevent the clients from collectively jittering around
on minor differences during playback. While still allowing
large, deliberate jumps from one client to still be propagated
out to all clients. No attempt is made to prevent jittering from
being visible in the Editor however, adding a sort of visual
diagnostic of when Clients are either in disagreement on rows per
second (bpm * rows-per-beat), or some bug or other source of
latency causing disagreement in playback.

When the Editor is paused however, any incoming SET_ROW is simply
broadcast, even the slightest difference. As these are assumed
to be explicit precise movements during the creative editing
process.

The impetus for adding this feature is supporting use cases like
a tracker that has been modified to send PAUSE and SET_ROW for
the current pattern editor's cursor row up to RocketEditor, for
coordinating visuals with music during the creative process, with
or without the demo showing visuals concurrently connected.

I have an experimental branch where SchismTracker already does
this, so this has actually been somewhat successfully tested, on
Linux at least.

Note I've also added the TCP_NODELAY socket flag, which is a
change I've already had merged at the core Rocket project as
well. When TCP_NODELAY is set everywhere in these multiclient
scenarios, it significantly helps keeping the jitter down.

The TCP_NODELAY addition may need some testing/changes for
handling non-Linux platforms, since that's all I've tested.

@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 25, 2023

I'll see about getting my local branch of SchismTracker in more presentable form so others can test this out at least as much as I've been able to in this experiment.

Care has been taken to not change the underlying Rocket protocol in service of multiple clients. It seems perfectly viable as-is in the single-demo scenario I've handled here, where all the other clients don't play with track data, instead being only interested in controlling playback pause/unpause and updating the current row.

I mostly just wanted to get this up to give others time to get on the same page and solicit feedback before I invest much more time on the SchismTracker side. Things are already functional enough for our private usage. But if this lands upstream there's incentive to clean up the Schism stuff and try get that merged upstream too for other demosceners to make use of. I do have an existing rapport with SchismTracker maintainers having done much of the SDL2 port work and various bugfixes over the years. So there's a good chance I can get Rocket support merged there.

Let me know if this doesn't make any sense, I can at least make a quick screencap demonstrating what's up. Imagine unified demo creating with RocketEditor alongside SchismTracker, modifying the tracked music for the demo live while modifying Rocket tracks. You move the cursor in the SchismTracker pattern data, and the row immediately moves in RocketEditor, keeping the edited rows synchronized for you. All this while potentially having the Demo window displaying the current output for Rocket's track values as well...

@emoon
Copy link
Owner

emoon commented Feb 25, 2023

Hi,

Nice. This is a pretty cool idea. Do you think you could add a section in the README also to describe this feature?

@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 25, 2023

Hi,

Nice. This is a pretty cool idea. Do you think you could add a section in the README also to describe this feature?

Thanks!

I was thinking it made more sense to hold off any mention in the docs until there's at least one existing tracker to mention as supporting it for interested users to kick the tires...

The existing single-client use case should be unchanged.

@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 25, 2023

Non-linux CI failures appear unrelated, and unfortunate since those are the OSes I haven't built on... would be nice to see those succeeding or providing meaningful feedback for fixups to this PR.

vcaputo added a commit to vcaputo/schismtracker that referenced this pull request Feb 26, 2023
This adds a --with-rocket configure flag for enabling
experimental GNU Rocket support in Schism.

When enabled in the build, a series of new runtime flags are
added:

--rocket (enable rocket mode)
all these overrides imply --rocket:
--rocket-host=HOST_OVERRIDE
--rocket-port=PORT_OVERRIDE
--rocket-bpm=BPM_OVERRIDE (beats per minute)
--rocket-rpb=RPB_OVERRIDE (rows per beat)

BPM and RPB are RocketEditor concepts, which the GNU Rocket
protocol is totally ignorant of.  But they're provided to aid in
keeping the Editor and Schism in agreement on what Rocket row
maps to what millisecond within the song and within the Rocket
track data.  If a RocketEditor user leaves the defaults for
these, there's no need to use them as I've put the defaults here
too.  (Note I don't think RocketEditor currently exposes RPB
setting in its UI, but it's in the code).  It's likely folks will
just ignore these two flags successfully using the defaults.

As-is this commit only attempts to connect to the Editor once at
startup.  So you'll need to have RocketEditor running before
starting SchismTracker w/--rocket.  This area could be made more
robust before merging upstream into SchismTracker, but it is
already usable.  If viewed as an experimental demo developer
mode, this is probably good enough to merge as-is.

See emoon/rocket#165 for context on what
this is all about.
@emoon
Copy link
Owner

emoon commented Feb 26, 2023

Non-linux CI failures appear unrelated, and unfortunate since those are the OSes I haven't built on... would be nice to see those succeeding or providing meaningful feedback for fixups to this PR.

I will fix these now.

@emoon
Copy link
Owner

emoon commented Feb 26, 2023

CI issues fixed, but you will need to pull latest from master so your branch get the changes as well.

@vcaputo vcaputo force-pushed the multiclient branch 2 times, most recently from 439e7c5 to c5996dd Compare February 27, 2023 18:47
@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 27, 2023

Huh, I expected the TCP_NODELAY changes to require some further work/ifdef fuckery to be happy on windows but CI is all green.

@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 28, 2023

Added blurb to README now that there's at least a draft PR to reference on schismtracker curious/courageous users can build and poke at the feature with.

@emoon
Copy link
Owner

emoon commented Feb 28, 2023

Cool. Do you think this PR is ready to be merged then?

@vcaputo
Copy link
Contributor Author

vcaputo commented Feb 28, 2023

Lemme get more testing in. It certainly works in quick experiments with my schism branch and a demo I'm working on, but I haven't done much testing of long-running sessions with lots of demo connects/disconnects and such.

I'll comment when it seems at least very unlikely to regress anything...

It'd be nice if someone on windows could hammer on it there too.

@emoon
Copy link
Owner

emoon commented Feb 28, 2023

Cool. Yeah, I don't run Windows as my primary desktop either...

@vcaputo
Copy link
Contributor Author

vcaputo commented Mar 1, 2023

Found and fixed some bugs, but things seem to work reliably now even with demo client repeatedly coming and going... Also did some tests with two simultaneous schismtracker instances, all more or less stay coordinated on the current row. Occasionally things get jittery, maybe that can be improved, but I don't think that's any reason to prevent merging.

One oddity I noticed is if the demo client doesn't request any tracks w/GET_TRACK, the connection status line will say "No demo connected, +N client(s)" ... since the demo client is identified via a "GET_TRACK performed" heuristic.

It's probably not a problem since why would you have a demo talking to the sync tracker without any tracks. But in my case I have a sort of meta-demo I'm working on where it's more a generalized collection of effects and compositor, you instantiate scenes at runtime which may then add sync tracks. So until a scene with tracks actually gets created, RocketEditor displays there's no demo connected (but does still account for the +N client, so it's not like you don't see that something's connected on behalf of the demo client)

Any remaining bugs should be minor and can be fixed as discovered post-merge...

vcaputo added a commit to vcaputo/schismtracker that referenced this pull request Jul 20, 2023
This adds a --with-rocket configure flag for enabling
experimental GNU Rocket support in Schism.

When enabled in the build, a series of new runtime flags are
added:

--rocket (enable rocket mode)
all these overrides imply --rocket:
--rocket-host=HOST_OVERRIDE
--rocket-port=PORT_OVERRIDE
--rocket-bpm=BPM_OVERRIDE (beats per minute)
--rocket-rpb=RPB_OVERRIDE (rows per beat)

BPM and RPB are RocketEditor concepts, which the GNU Rocket
protocol is totally ignorant of.  But they're provided to aid in
keeping the Editor and Schism in agreement on what Rocket row
maps to what millisecond within the song and within the Rocket
track data.  This doesn't override the speed/tempo setting for
the song within Schism, and you'll have to take care to pick
those values such that it's consistent with the Rocket values, if
you want the RocketEditor's patterns and current row to align
with Schisms.  It's a little awkward due to how IT defined the
speed/tempo semantics.  The default Rocket values of 125BPM/8RPB
do seem to align with the default IT/Schism speed/tempo of 6/125
though.

As-is this commit only attempts to connect to the Editor once at
startup.  So you'll need to have RocketEditor running before
starting SchismTracker w/--rocket.  This area could be made more
robust before merging upstream into SchismTracker, but it is
already usable.  If viewed as an experimental demo developer
mode, this is probably good enough to merge as-is.

See emoon/rocket#165 for more context on
what this is all about.
vcaputo added a commit to vcaputo/schismtracker that referenced this pull request Jul 21, 2023
This adds a --with-rocket configure flag for enabling
experimental GNU Rocket support in Schism.

When enabled in the build, a series of new runtime flags are
added:

--rocket (enable rocket mode)
all these overrides imply --rocket:
--rocket-host=HOST_OVERRIDE
--rocket-port=PORT_OVERRIDE
--rocket-bpm=BPM_OVERRIDE (beats per minute)
--rocket-rpb=RPB_OVERRIDE (rows per beat)

BPM and RPB are RocketEditor concepts, which the GNU Rocket
protocol is totally ignorant of.  But they're provided to aid in
keeping the Editor and Schism in agreement on what Rocket row
maps to what millisecond within the song and within the Rocket
track data.  This doesn't override the speed/tempo setting for
the song within Schism, and you'll have to take care to pick
values such that they align with the Rocket bpm/rpb values.

If you want the RocketEditor's patterns and current row to align
with Schisms.  It's a little awkward due to how IT defined the
speed/tempo semantics.  According to IT.TXT these are the rules
for Axx/Txx (speed/tempo):

 Axx   Set Speed.

       I prefer to think of this command as "Set Frames per Row".
       Normally, the tracker operates at around 50 frames a
       second. If the rows were played at this speed, then a huge
       amount of space would be required to enter the pattern data.
       Instead, setting the 'speed' of the song will cause the
       tracker to wait on the current row for 'xx' frames. Hence,
       setting the speed at 50 (decimal = 32hex) will cause each
       row to last about a second - quite a long time! The default
       is A06. The initial speed can be set in the variables
       screen on F12.

 Txx   Set tempo to xx

        Valid ranges are between 20h and 0FFh. The higher the
        value, the faster the playback. This essentially
        determines the time length of each frame, by the following
        formulas:
                      Frames per minute = 24*Tempo
        equivalently:
                      Frames per second = 0.4*Tempo

Which means you can determine the appropriate --rocket-bpm via:
24*Txx/Axx/RPB

So the schism default of 6/125:
24*125/6 = 500 rows per minute
with an rpb of 4, the bpm would be 125

What's an appropriate --rocket-rpb depends on if you want equal,
more, or less resolution in the rocket tracks than the schism
pattern data.  It's convenient to make them map 1:1, but if you
use a higher --rocket-rpb that's a multiple of 1:1 then you get
more Rocket rows per Schism row.  Which can be necessary for
getting enough precision in scheduling visual events if Axx is
high (a lot of time is burned per schism row).

Another complication here is dynamic Axx/Txx changes in the song.
No attempt is made to change the --rocket-rpb/bpm values mid-song
or anything.  You just set it once @ startup and that's a
constant.  So you probably want to avoid using those in demo
songs, or be prepared to deal with the wonky alignments in the
RocketEditor.

As-is this commit only attempts to connect to the Editor once at
startup.  So you'll need to have RocketEditor running before
starting SchismTracker w/--rocket.  This area could be made more
robust before merging upstream into SchismTracker, but it is
already usable.  If viewed as an experimental demo developer
mode, this is probably good enough to merge as-is.

See emoon/rocket#165 for more context on
what this is all about.
vcaputo added a commit to vcaputo/schismtracker that referenced this pull request Jul 21, 2023
This adds a --with-rocket configure flag for enabling
experimental GNU Rocket support in Schism.

When enabled in the build, a series of new runtime flags are
added:

--rocket (enable rocket mode)
all these overrides imply --rocket:
--rocket-host=HOST_OVERRIDE
--rocket-port=PORT_OVERRIDE
--rocket-bpm=BPM_OVERRIDE (beats per minute)
--rocket-rpb=RPB_OVERRIDE (rows per beat)

BPM and RPB are RocketEditor concepts, which the GNU Rocket
protocol is totally ignorant of.  But they're provided to aid in
keeping the Editor and Schism in agreement on what Rocket row
maps to what millisecond within the song and within the Rocket
track data.  This doesn't override the speed/tempo setting for
the song within Schism, and you'll have to take care to pick
values such that they align with the Rocket bpm/rpb values.

If you want the RocketEditor's patterns and current row to align
with Schisms.  It's a little awkward due to how IT defined the
speed/tempo semantics.  According to IT.TXT these are the rules
for Axx/Txx (speed/tempo):

 Axx   Set Speed.

       I prefer to think of this command as "Set Frames per Row".
       Normally, the tracker operates at around 50 frames a
       second. If the rows were played at this speed, then a huge
       amount of space would be required to enter the pattern data.
       Instead, setting the 'speed' of the song will cause the
       tracker to wait on the current row for 'xx' frames. Hence,
       setting the speed at 50 (decimal = 32hex) will cause each
       row to last about a second - quite a long time! The default
       is A06. The initial speed can be set in the variables
       screen on F12.

 Txx   Set tempo to xx

        Valid ranges are between 20h and 0FFh. The higher the
        value, the faster the playback. This essentially
        determines the time length of each frame, by the following
        formulas:
                      Frames per minute = 24*Tempo
        equivalently:
                      Frames per second = 0.4*Tempo

Which means you can determine the appropriate --rocket-bpm via:
24*Txx/Axx/RPB

So the schism default of 6/125:
24*125/6 = 500 rows per minute
with a "rows per beat" of 4, the bpm would be 125

What's an appropriate --rocket-rpb depends on if you want equal,
more, or less resolution in the rocket tracks than the schism
pattern data.  It's convenient to make them map 1:1, but if you
use a higher --rocket-rpb (preferably that's a multiple of the
rpb you're using in schism patterns), then you get more Rocket
rows per Schism row.  This can be necessary for getting enough
precision in scheduling visual events, especially if Axx is high
(more time is burned per schism row the higher Axx is).

Another complication here is dynamic Axx/Txx changes in the song.
No attempt is made to change the --rocket-rpb/bpm values mid-song
or anything.  You just set it once @ startup and that's a
constant.  So you probably want to avoid using those in demo
songs, or be prepared to deal with the wonky alignments in the
RocketEditor.

As-is this commit only attempts to connect to the Editor once at
startup.  So you'll need to have RocketEditor running before
starting SchismTracker w/--rocket.  This area could be made more
robust before merging upstream into SchismTracker, but it is
already usable.  If viewed as an experimental demo developer
mode, this is probably good enough to merge as-is.

See emoon/rocket#165 for more context on
what this is all about.
These aren't accessed externally, make explicit.
Mechanical rename to reduce ambiguity in a future having multiple
clients.  This doesn't index clients, it indexes the track
mapping for the demo client...
This adds the ability for RocketEditor to accept multiple
simultaneous Rocket connections.

The connection which performs GET_TRACK is assumed to be the
Demo, which is a privileged client owning the map of track
names<->IDs.  If another client attempts to GET_TRACK or SET_KEY
than the current Demo client, that client is diconnected and an
error logged.  This single-demo limitation greatly simplifies
supporting multiple clients, as it skirts trying to turn the
track-ID mapping into more of a distributed mapping within the
existing Rocket protocol.

All clients however are permitted to send SET_ROW and PAUSE, with
these commands also being broadcast (with filtering) to all
clients for keeping them synchronized.

There's some ickiness to handling SET_ROW during playback, since
it's unlikely the clients are all *perfectly* aligned on their
current row.

When unpaused, incoming SET_ROW values are filtered to always
make forward progress.  This prevents the editor from jittering
backwards during playback due to minor discrepancies in the
clients current row.  The exception to this is when the delta is
at least a second worth of rows, which is seen as a deliberate
jump, regardless of direction.  The filtered SET_ROW values are
also prevented from propagating to other clients.

When paused however, any incoming SET_ROW is simply broadcast,
even the slightest difference.  As these are assumed to be
explicit precise movements during the creative track editing
process.  When broadcasting a SET_ROW, the originating client is
filtered from the recipients.  So clients don't need to do
anything WRT handling an echo.

The impetus for adding this feature is supporting use cases like
a tracker that has been modified to send PAUSE, and SET_ROW for
the pattern editor's cursor row up to RocketEditor.  This is
useful in coordinating visuals with music during the creative
process, with or without the demo showing visuals simultaneously
connected via Rocket.

I have an experimental branch where SchismTracker already does
this, so this has actually been somewhat successfully tested, on
Linux at least.

Note I've also added the TCP_NODELAY socket flag, which is a
change I've already had merged at the core Rocket project as
well.  When TCP_NODELAY is set everywhere in these multiclient
scenarios, it significantly helps keeping the jitter down.

The TCP_NODELAY addition may need some testing/changes for
handling non-Linux platforms, since that's all I've tested.
Includes link to my SchismTracker draft PR adding rudimentary
Rocket support, since it's unclear when if ever that will get
merged upstream.
@vcaputo
Copy link
Contributor Author

vcaputo commented Aug 16, 2023

@emoon I wonder if this can be merged now?

@emoon emoon merged commit 6aa0f6f into emoon:master Aug 17, 2023
3 checks passed
@emoon
Copy link
Owner

emoon commented Aug 17, 2023

Merged. Thanks for the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants